Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cmd] Define order of parallel groups #6602

Merged
merged 6 commits into from
Jun 1, 2024

Conversation

KangarooKoala
Copy link
Contributor

Using lists in Java parallel groups guarantees iteration order and also matches C++. (Although this uses two separate lists to avoid having to allocate Pair objects.)

Note that after this PR gets merged, #6599 should be updated to change the DeadlineFor, AlongWith, and RaceWith tests to use std::move(command).Decorate instead of std::move(command).ToPtr().Decorate. (Or after the other PR gets merged, this PR should be updated to use the new decorators.)

@KangarooKoala KangarooKoala requested a review from a team as a code owner May 9, 2024 02:50
@Starlight220
Copy link
Member

Although this uses two separate lists to avoid having to allocate Pair objects.

This feels risky; too much chance of the state falling out of sync.

Is there no map that preserves insertion order?

If C++ uses pairs then maybe Java should too (use Map.Entry).

@SamCarlberg
Copy link
Member

Although this uses two separate lists to avoid having to allocate Pair objects

Use LinkedHashMap, which preserves insertion order.

@KangarooKoala
Copy link
Contributor Author

Although this uses two separate lists to avoid having to allocate Pair objects

Use LinkedHashMap, which preserves insertion order.

I would use LinkedHashMap and LinkedHashSet (even did that on 61870b4), but I'm concerned about discrepancies between Java and C++ (Given how many weird functional differences have ended up popping up). Now that I think about it though, it would be okay to use something like List<Pair> (but not the specific WPILib class since that's immutable), since in the previous code we iterated over the entrySet() (which would require making Map.Entry objects anyways). I personally prefer making a small private static inner class since using List<Map.Entry> feels like an misuse of Map.Entry, but if people have reasons to go with that or a different route, I'm open to hearing suggestions. (It'll probably be a bit before I can start coding anyways)

I'll also point out that in CommandScheduler.java#L96 we use two associated lists to avoid making pair objects, so a separate PR should probably make the same change as whatever we decide for this PR.

@kytpbs
Copy link
Contributor

kytpbs commented May 12, 2024

I would use LinkedHashMap and LinkedHashSet (even did that on 61870b4), but I'm concerned about discrepancies between Java and C++

LinkedHashMap behaves in the same way as a linked list so I don't think it should be a problem?

@KangarooKoala
Copy link
Contributor Author

After thinking about this some more, I've decided that it makes sense for this PR to make the minimal changes to define the order of iteration (so yes I'll be using LinkedHashMap and LinkedHashSet), and in another PR (after this one is merged) we'll make the parallel group implementations match (Using lists of pair objects, keeping track of a m_finished variable, and maybe some other stuff I haven't noticed yet).
(I still think that Java and C++ should be as close to 1 to 1 copies as possible, but I've realized that doing those changes in this PR is a bit of scope creep)

Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done on the detailed comments!

@SamCarlberg
Copy link
Member

FYI there will be conflicts with #6032 depending on merge order

@PeterJohnson PeterJohnson merged commit 1828fda into wpilibsuite:main Jun 1, 2024
25 checks passed
@KangarooKoala KangarooKoala deleted the parallel-order branch June 1, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants