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

Initial rayon support: ParallelExtend + FromParallelIterator #89

Merged
merged 3 commits into from
Jun 15, 2020

Conversation

Others
Copy link
Contributor

@Others Others commented May 26, 2020

This was a straightforward step towards #14.

I'm interested in trying to get some parallel iterator work going, but I thought I'd start with some small stuff :)

Points that need addressing:

  • par_extend_sync is a crap name, so we likely need to think of something better!
  • we create a guard for every insert operation which sucks, and I wonder if we can do better
    (this is especially relevant since I believe there's going to be a similar problem with parallel iterator support)

This change is Reviewable

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #89 into master will decrease coverage by 1.59%.
The diff coverage is 0.00%.

Impacted Files Coverage Δ
src/lib.rs 100.00% <ø> (ø)
src/map_ref.rs 87.03% <ø> (+9.67%) ⬆️
src/rayon_impls.rs 0.00% <0.00%> (ø)
src/set.rs 94.02% <ø> (+35.29%) ⬆️
src/set_ref.rs 45.23% <ø> (+45.23%) ⬆️
src/node.rs 73.08% <0.00%> (-4.89%) ⬇️
src/raw/mod.rs 85.43% <0.00%> (-3.89%) ⬇️
src/map.rs 81.48% <0.00%> (-2.62%) ⬇️
src/iter/traverser.rs 91.20% <0.00%> (-0.80%) ⬇️
... and 3 more

@jonhoo
Copy link
Owner

jonhoo commented May 26, 2020

Thank you!

@cuviper Since you have the most rayon experience (I believe), would you mind taking a look at this?

@Others
Copy link
Contributor Author

Others commented May 26, 2020

@jonhoo No problem. Do you have any suggestions for getting my tests running on CI? They’re behind a feature flag so I don’t think they’re being picked up.

@jonhoo
Copy link
Owner

jonhoo commented May 26, 2020

By default the normal test suite should be run with --all-features I believe. If you want them to also run with miri/ASAN/LSAN, you'll want to modify these lines:

- script: cargo miri test -- -Zmiri-ignore-leaks

env ASAN_OPTIONS="detect_odr_violation=0" RUSTFLAGS="-Z sanitizer=address" cargo test --lib --tests --features sanitize --target x86_64-unknown-linux-gnu

env RUSTFLAGS="-Z sanitizer=leak" cargo test --features sanitize --target x86_64-unknown-linux-gnu

To also include your feature (or just make them --all-features — that seems reasonable too).

@Others
Copy link
Contributor Author

Others commented May 26, 2020

Ah, I guess they're running but not picked up by codecov?
I'm assuming MIRI isn't going to work, as the tests use threading. I'll add --all-features to the sanitizer runs.

I've been thinking a bit about having to create a guard for every element we want to add. Creating a guard is a bit expensive (it definitely has a performance impact on my project using flurry) and might be dragging down performance. We could consider using thread-local to optimize into just creating one guard per worker thread. Whoever reviews this might want to consider whether that optimization is worth it.

@jonhoo
Copy link
Owner

jonhoo commented May 26, 2020

Ahhh, yeah, that's awkward. Looks like the template we're using doesn't pass --all-features to tarpaulin. Easiest way to fix that is probably to just inline the code to run it. We won't need the templating stuff or any of the token detection stuff, so only the last job should be relevant.

So, the guard stuff is actually pretty thorny 😅 We've discussed it a lot over in jonhoo/bustle#2 (comment) (and the following comments) which you may want to take a look at. I think we should probably not make a change to how guards work in this PR. Definitely agree that we should be able to do better than creating a guard for every access in rayon though. @cuviper does rayon have a way to do some "prep" on each thread that is running the parallel iterator?

@Others
Copy link
Contributor Author

Others commented May 26, 2020

Yeah my initial idea to use a thread local on the stack doesn't work, as Guard is not Send. (Which makes sense, since it pins a thread.)

src/rayon_impls.rs Show resolved Hide resolved
src/rayon_impls.rs Outdated Show resolved Hide resolved
src/rayon_impls.rs Outdated Show resolved Hide resolved
@Others Others requested a review from cuviper May 27, 2020 03:46
Copy link
Collaborator

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

The code looks good to me.

I'll let @jonhoo decide if CI/coverage is good enough...

@Others
Copy link
Contributor Author

Others commented Jun 8, 2020

Just pinging that this is still on my radar, will hopefully get to this in the later half of the week :)

@Others
Copy link
Contributor Author

Others commented Jun 15, 2020

Okay I think this hits a decent subset of the impls we want, plus mostly okay tests. Might be suffering a bit of PR creep, so I'd rather not make it any bigger.

@Others Others requested a review from jonhoo June 15, 2020 01:14
@jonhoo
Copy link
Owner

jonhoo commented Jun 15, 2020

This looks good to me — thanks!

@jonhoo jonhoo merged commit f623ac9 into jonhoo:master Jun 15, 2020
@cheako
Copy link

cheako commented Apr 17, 2021

One of Rust's features is the naming convention, it seems clumsy and random(like a blaster). That has both pro(s) and con(s), like not knowing what something is. I think the OP(@Others @jonhoo) here should be extended with a short description of what is the goal this PR is trying to accomplish... With an intention on explaining it to someone who will then follow newly added links to rayon site(s) for deeper explanation and further reading I.E. someone who thinks rayon has more todo with light than as a Rust crate.

Walk entries on multiple threads using convenience [rayon](https://docs.rs/rayon) wrapper [trait](https://docs.rs/rayon/latest/rayon/iter/trait.ParallelIterator.html).  Also consume same trait for [insertion](https://docs.rs/rayon/latest/rayon/iter/trait.ParallelExtend.html) on multiple threads.

@jonhoo
Copy link
Owner

jonhoo commented Apr 18, 2021

I'm not exactly sure what you're asking for here? If you'd like to see documentation on a particular method be improved, then I'd be happy to take a PR with that improvement!

@cheako
Copy link

cheako commented Apr 18, 2021

The goal was unclear without prior knowledge of rayon or whatever. Without links to rayon that leads to googling or looking to see if there is a docs page.

@jonhoo
Copy link
Owner

jonhoo commented Apr 20, 2021

The goal of what exactly? Of the PR? Of the commit? Or of the interface in the code? I don't generally expect contributors to document their PRs/commits directly, since it's assumed that those will be seen by developers of the library, not of its users.

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.

4 participants