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

Optimize jobserver try_acquire #1037

Merged
merged 8 commits into from Apr 20, 2024
Merged

Optimize jobserver try_acquire #1037

merged 8 commits into from Apr 20, 2024

Conversation

NobodyXu
Copy link
Collaborator

@NobodyXu NobodyXu commented Apr 18, 2024

First try jobserver::Client::try_acquire (available on jobserver 1.0.29), which will work:

  • If a fifo is used as jobserver
  • On linux and:
    • preadv2 with non-blocking read available (>=5.6)
    • /proc is available
  • On Windows
  • On wasm

if not, we will simply fallback to help thread implementation, spawning one
thread to maintain compatibility with other platforms.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
First try `jobserver::Client::try_acquire`, which will work:
 - If a fifo is used as jobserver
 - On linux and:
    - preadv2 with non-blocking read available (>=5.6)
    - /proc is available
 - On Windows
 - On wasm

if not, we will simply fallback to help thread implementation, spawning one
thread to maintain compatibility with other platforms.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Also impls `Send`, `Sync`, `RefUnwindSafe` and `UnwindSafed` when the `T`
meets the criterior.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
whenever feature parallel is enabled.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
There is no need to, it never fails.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu requested a review from thomcc April 18, 2024 11:55
@workingjubilee
Copy link

I do not believe it is acceptable to vendor OnceLock. It is a bad idea for a low-maintenance crate to vendor code that may need a security update.

@NobodyXu
Copy link
Collaborator Author

I do not believe it is acceptable to vendor OnceLock. It is a bad idea for a low-maintenance crate to vendor code that may need a security update.

Thanks for pointing out, under @the8472 's advice, I've replaced that with once_cell dependency and removed the vendoring of OnceLock.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This seems reasonable.

src/parallel/job_token.rs Show resolved Hide resolved
@NobodyXu NobodyXu merged commit 9af6b95 into main Apr 20, 2024
46 checks passed
@NobodyXu NobodyXu deleted the bump/jobserver branch April 20, 2024 03:08
BurntSushi added a commit to BurntSushi/ripgrep that referenced this pull request Apr 23, 2024
Notably, this removes winapi in favor of windows-sys, as a result of
winapi-util switching over to windows-sys[1].

Annoyingly, when PCRE2 is enabled, this brings in a dependency on
`once_cell`[2]. I had worked to remove it from my dependencies and now
it's back. Gah. I suppose I could disable the `parallel` feature of
`cc`, but that doesn't seem like a good trade-off.

[1]: BurntSushi/winapi-util#13
[2]: rust-lang/cc-rs#1037
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.

None yet

3 participants