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

Fix Sync issues with FuturesUnordered #2054

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

okready
Copy link
Contributor

@okready okready commented Jan 28, 2020

Fixes #2050.

This contains a couple possible fixes for Sync trait correctness with FuturesUnordered, in particular with regards to simultaneous FuturesUnordered::push and FuturesUnordered::iter calls. These fixes are currently split up into two commits for review.

The base fix replaces head_all and len updates in FuturesUnordered::link with atomic operations to ensure the linked list state is valid if FuturesUnordered::push is called from multiple threads simultaneously. Since keeping the two fields in-sync would likely require locking, the fields are still updated separately, but the final results should still be correct. Unfortunately, this means that creating an iterator with FuturesUnordered::iter cannot rely on the len value it reads being correct for whatever is read for head_all. Since IterPinRef implements ExactSizeIterator, we need a valid length to avoid breaking compatibility, so the length is lazily initialized by counting the number of futures manually when IterPinRef::size_hint is first called, which can be costly for large FuturesUnordered sets.

The second commit changes how FuturesUnordered lengths are stored to avoid the potentially high cost of manually counting futures. Instead of storing and updating a single len value in the FuturesUnordered itself, each Task now stores a snapshot of the list length when the Task was inserted, avoiding the need to count list nodes or synchronize separate len and head_all reads, although at the cost of an additional usize per Task.

In mutable contexts, atomic operations are either performed exclusively using "relaxed" ordering semantics or are avoided altogether, as we can be sure that access to the FuturesUnordered list has already been synchronized and that no threads can modify head_all or any fields related to that list (only the ReadyToRunQueue and its link pointers can be updated from other threads while in a mutable function).

I can squash the commits or drop the second one and clean up the PR if either of these fixes are okay.

@cramertj
Copy link
Member

Nice! Would you mind running the FuturesUnordered benchmarks? If both of those look reasonable, I'd say let's merge both commits.

@okready okready force-pushed the futures-unordered-sync-fixes branch from 4d1d043 to 6421819 Compare January 29, 2020 05:53
@okready
Copy link
Contributor Author

okready commented Jan 29, 2020

Here's what I got on an x86-64 machine (I haven't taken a look at the impact on an ARM or other weak memory model system):

Current master: 3,305,925 ns/iter (+/- 30,292)
With changes: 3,437,795 ns/iter (+/- 34,100)

I've flattened the two commits, and also made an adjustment to the next_all spin-wait in IterPinRef where I realized Acquire ordering wasn't necessary (the Acquire ordering when initially reading head_all is sufficient to cover all nodes that were inserted prior to the current head).

This updates non-mutable FuturesUnordered methods to be thread-safe,
ensuring that Sync trait requirements are met without breaking existing
functionality.

The FuturesUnordered::link method used by FuturesUnordered::push has
been modified to use atomic operations to allow for lock-free insertion
of new futures while still allowing FuturesUnordered::iter and
IterPinRef itself to operate simultaneously. FuturesUnordered::len has
been removed, with each Task now storing a snapshot of the list length
at the time when the future was added, so a synchronized list length can
always be read after an atomic load of FuturesUnordered::head_all. This
increases the memory overhead of each task slightly, but it allows us to
retain the original iterator behavior without dramatically increasing
the performance cost.

Operations that require mutable FuturesUnordered access have not been
made lock-free, with direct mutable access to otherwise atomic values
used when possible to avoid unnecessary overhead.
@okready okready force-pushed the futures-unordered-sync-fixes branch from 6421819 to 2d73173 Compare January 29, 2020 08:46
@cramertj
Copy link
Member

Thanks so much, looks great! I'll get a release out shortly.

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.

FuturesUnordered has an incorrect Sync impl
2 participants