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

New lint: loop with atomics without other side effects #7809

Closed
Kixunil opened this issue Oct 12, 2021 · 2 comments · Fixed by #8174
Closed

New lint: loop with atomics without other side effects #7809

Kixunil opened this issue Oct 12, 2021 · 2 comments · Fixed by #8174
Labels
A-lint Area: New lints

Comments

@Kixunil
Copy link

Kixunil commented Oct 12, 2021

What it does

The lint would check that a loop has no side effects other than atomic access and warn to park the thread or call std::hint::spin_loop().

Categories (optional)

  • Kind: perf

What is the advantage of the recommended code over the original code

If the thread is parked it will stop consuming resources and avoid priority inversion.
If spin_loop() hint is used the code will consume less power on some architectures or perform better.
Presence of spin_loop() could also better communicate to reviewer that author did not forget to park the thread.

Drawbacks

None that I know of but I'm not very knowledgeable about the topic.
In some cases it may be better to use futex instead of thread::park().

Example

while some_atomic.load(atomic::Ordering::Acquire) == NOT_READY {}

Could be written as:

while some_atomic.load(atomic::Ordering::Acquire) == NOT_READY {
    std::hint::spin_loop();
}

or

while some_atomic.load(atomic::Ordering::Acquire) == NOT_READY {
    thread::park();
}

with thread::unpark() at the place where a value is stored into atomic

Additional context

Loosely related: rust-lang/miri#1388 (comment)

@Kixunil Kixunil added the A-lint Area: New lints label Oct 12, 2021
@camsteffen
Copy link
Contributor

I suspect there are other similar patterns. Lint name idea: missing_spin_loop.

@Kixunil
Copy link
Author

Kixunil commented Oct 12, 2021

Yeah, should cover CAS etc too. I think a clearer name would be missing_spin_loop_hint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants