wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.05k stars 611 forks source link

[cmd] Define order of parallel groups #6602

Closed KangarooKoala closed 3 months ago

KangarooKoala commented 3 months ago

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.)

Starlight220 commented 3 months ago

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 commented 3 months ago

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

Use LinkedHashMap, which preserves insertion order.

KangarooKoala commented 3 months ago

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 commented 3 months ago

I would use LinkedHashMap and LinkedHashSet (even did that on https://github.com/wpilibsuite/allwpilib/commit/61870b44f3842fe635d0683f51ee06efd4271253), 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 commented 3 months ago

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)

SamCarlberg commented 3 months ago

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