Add more clarification to spin_loop docs

[clarfonthey]

Follow-up to #159121.

Attempting to update the spin_loop docs to make it a little more clear what the use case for this hint is. Also uses the term "hardware thread" since "hyper-thread" is an Intel-specific term.

[rustbot]

r? @aapoalas

rustbot has assigned @aapoalas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[hanna-kruppe]

I think talking about "expected wait time" here is a red herring.

• If you're waiting on another thread, then there is generally no way to get a useful bound on how long it might take until that other thread releases the lock because that's up to the OS scheduler. Especially if you're spinning and thus potentially preventing the other thread from being scheduled. Critical section length in instructions or clock cycles has nothing to do with it.
• If you're doing a non-blocking CAS or LL/SC loop (like Atomic*::update is intended for), then the duration of the operation that the loop tried to does matter, but only because longer update sequences marginally increase the likelihood and cost of contention. In any case, such loops shouldn't use spin_loop in the first place (as the rest of this paragraph explains).

View changes since the review

[hanna-kruppe]

Conversely: some mutex implementations (including std's futex-based one) happily do the optimistic N-iterations-of spinning even though they have no idea how long the critical sections protected by any particular mutex are. That's fine because it's a still a win when it works out, not too costly compared to the syscall when it doesn't work out, and the fallback to the syscall is still needed in any case.

[hanna-kruppe]

I see the "only when expected wait time is short" point remained; I still think it's a red herring that distracts from the other (correct) points being made. In addition, many people have already internalized the folklore that "spin locks are great for short critical sections" and I'm worried about implicitly reinforcing it by mentioning something that sounds related.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[aapoalas]

r? @hanna-kruppe

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[clarfonthey]

So, finally getting back to this after sitting on it for a while. I agree with everything you said, but since documentation for atomics is often very technical and poorly explained, I wanted to try and come up with something a bit more digestible without compromising accuracy.

Hopefully the new version covers the feedback you gave a bit better.

[clarfonthey]

@rustbot review

[hanna-kruppe]

Thanks, I like this direction and agree it helps make it more digestible. There's still two points (one new, one old) where I think this is inaccurate and could encourage a wrong mental model.

@rustbot author

View changes since this review

[hanna-kruppe]

I can see the argument for trying to tie this point to"improper usage of spin_loop" as that's the method being documented here. But I think it's a bit misleading to attribute the problem to using spin_loop rather than to busy-waiting at all, especially when immediately followed by an example that illustrates a different reason to not use spin_loop. It would be unfortunate if someone read this and concluded that they should remove spin_loop from their spin-lock-esque loop.

An easy fix would be to move the mention of priority inversion to the paragraph below that talks about use in mutexes. But if you're up for it, I could see splitting this up into two separate examples:

1. Show a very simple busy-looping spin lock that uses spin_loop (just the inner loop trying to acquire the lock, doesn't need to be runnable), and mention the problems with that¹. This would go together with the discussion of how proper lock implementations use spin_loop and why they still yield to the OS after some attempts.
2. Explain that this is only for busy-wait loops that wait on another thread, using the Atomic::update example and existing discussion.

Footnotes

1. Priority inversion for one, but if you're up for it, it would also be great to mention the terrible performance even for same-priority threads if the thread that has acquired the lock is not currently running. ↩

[hanna-kruppe]

nit: Atomic and the generic methods on it are still unstable, so linking there is a bit weird for users of stable. Maybe just link AtomicU32::update specifically?

[clarfonthey]

Yeah, was thinking about that and not sure. I can go with the u32 version

[hanna-kruppe]

I see the "only when expected wait time is short" point remained; I still think it's a red herring that distracts from the other (correct) points being made. In addition, many people have already internalized the folklore that "spin locks are great for short critical sections" and I'm worried about implicitly reinforcing it by mentioning something that sounds related.