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

Run formatters in parallel #17

Merged
merged 2 commits into from
Jan 27, 2021
Merged

Run formatters in parallel #17

merged 2 commits into from
Jan 27, 2021

Conversation

basile-henry
Copy link
Contributor

Closes #15

All the formatters are run in parallel using rayon's powerful par_iter which uses a thread pool under the hood.

Should I setup a CLI flag/env var to specify the number of threads to use?

Currently errors are not reported, but I can implement this in this PR. Do we have a preferred way to report multiple errors? I'm not sure that's something anyhow exposes, and we might want to format the errors ourselves anyway.

All the formatters are run in parallel using rayon's powerful par_iter
which uses a thread pool under the hood.
@basile-henry basile-henry added the enhancement New feature or request label Jan 26, 2021
@basile-henry basile-henry self-assigned this Jan 26, 2021
@zimbatm zimbatm requested a review from Rizary January 26, 2021 22:31
@zimbatm
Copy link
Member

zimbatm commented Jan 26, 2021

That was easy! Got to love the rust library ecosystem.

The threads seem to default on the number of CPUs which is fine. We can always tune this later with some user feedback.

At some point, we'll need to forward the exit status to the parent process exit status if any of those fail, although it can happen at a later stage.

@Rizary
Copy link
Contributor

Rizary commented Jan 27, 2021

Thank you for helping us to implement this. It wasn't easy for me to choose the library (threadpool, rayon, crossbeam) for this issue. I've been researching which library to use while working on another issue. Have you tried crossbeam? In my research, crossbeam provides various queues that allow pushing and popping items from several threads without a mutex.

The code is LGTM. And I agree with what @zimbatm said, we can change it later if we need to.

Regarding the error, I haven't think of displaying multiple errors when the formatters run in parallel.

@basile-henry
Copy link
Contributor Author

Have you tried crossbeam?

Yes, and it's a great library. rayon effectively uses crossbeam under the hood. I haven't tried the threadpool crate, so I can't comment on that one (though rayon does offer a very similar threadpool API if we ever need it).

I think for this use case rayon's ParallelIterator is perfect. There are no dependencies between the tasks so we don't need to use channels explicitly and it makes for a short/concise implementation.

I haven't think of displaying multiple errors

I will try to come up with something 😄 (probably a different PR)
We need to think of where we want to see these errors: stdout/stderr/log file, and whether the --quiet flag affects it. We probably also want to specify which file/formatter is at the origin of the errors.

src/formatters/tool.rs Outdated Show resolved Hide resolved
@Rizary
Copy link
Contributor

Rizary commented Jan 27, 2021

rayon effectively uses crossbeam under the hood

Oh, this is great, I didn't notice it. We should stick with rayon then.

I think for this use case rayon's ParallelIterator is perfect. There are no dependencies between the tasks so we don't need to use channels explicitly and it makes for a short/concise implementation.

Yes, I agree with this.

I will try to come up with something 😄 (probably a different PR)

Yes, agree that it should be in a different PR.

@Rizary Rizary merged commit 2ec001c into master Jan 27, 2021
@basile-henry basile-henry deleted the basile/parallel-formatting branch January 27, 2021 10:38
brianmcgee pushed a commit that referenced this pull request May 13, 2024
Since we discussed this in https://git.numtide.com/numtide/treefmt/pulls/14#issuecomment-609

It doesn't really matter.

Reviewed-on: https://git.numtide.com/numtide/treefmt/pulls/17
Co-authored-by: zimbatm <[email protected]>
Co-committed-by: zimbatm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run formatters in parallel
3 participants