fix: don't sched_yield() on every object-lookup miss once all indices are loaded (#2878) - #3000
Arbel (arbelonson-source) wants to merge 2 commits into
Conversation
…es are loaded. (GitoxideLabs#2878) `load_next_index()` yielded unconditionally whenever there was no slot left to claim, before checking whether another thread was still loading an index. Once all indices are loaded, that is the path every `contains()` or `find()` miss takes, so each miss paid a syscall even with `RefreshMode::Never` and nothing loading at all. The yield papered over a race: the counter of indices being loaded was only incremented *after* a slot was claimed, so a thread that found nothing left to claim could observe the counter at zero while the claiming thread had not yet announced its load, and would return without all indices at its disposal. Now the counter is incremented *before* a slot is claimed, and decremented right away if the claim finds nothing to load. Every claimed slot is thus covered by an increment that precedes the claim (all accesses are `SeqCst`), so a thread that fails to claim and then reads the counter as zero knows no claimed slot is still loading, and the yield in front of the wait loop can go. Measured with 20 000 lookups of absent ids against a fully loaded store under `strace -c`: 20 001 `sched_yield` calls before, none after; the `statx` per miss from the loose-object check is unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0943f17ff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| // that happened before the claim, and the decrement only happens once loading is done (or failed). | ||
| // Hence a thread that finds nothing left to claim and observes this counter at zero afterwards knows | ||
| // that no claimed slot is still being loaded, without having to yield first to widen its window. | ||
| let ongoing_operation = IncOnNewAndDecOnDrop::new(&index.num_indices_currently_being_loaded); |
There was a problem hiding this comment.
When 65,536 load_next_index() calls overlap, this new placement counts every caller before it knows whether a slot can be claimed, including callers that will immediately take the miss path. AtomicU16::fetch_add then wraps to zero even if another caller is still loading an index, so a losing caller can skip the wait loop and return false with a snapshot that lacks that index. Use a non-wrapping-sized counter (or otherwise prevent overflow) for this expanded concurrency scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair point: with the increment moved in front of the claim, the bound on the counter is the number of threads in this path rather than the number of slots. Widened it to a usize in f4cadb1 (internal type only; tests, clippy and fmt re-run).
Since the previous commit every caller of `load_next_index()` touches the counter, briefly, before it knows whether it will load anything. Its upper bound is therefore the number of threads in that path rather than the number of slots, and with a `u16` an implausible but possible 65 536 concurrent callers would wrap it to zero and let a thread skip the wait. A `usize` cannot wrap in practice, and the counter is internal, so nothing else changes.
|
Superseded by #2853 . |
|
Understood, thanks. I searched PRs by the issue number and missed #2853, which predates it. Sorry for the duplicate. |
2 participants
Addresses #2878, where Rickard Ernst Björn Lundin (@Ignalina) measured a
sched_yieldon everycontains()miss once all indices are loaded, and asked whether the unconditionalyield_now()inload_next_index()is still needed.Why the yield was there
num_indices_currently_being_loadedwas incremented only after a slot had been claimed throughnext_index_to_load. A thread that found nothing left to claim could therefore observe the counter at zero while the claiming thread had not announced its load yet, break out of the wait loop, and hand its caller a snapshot that is missing an index. The unconditionalyield_now()widened that window enough to make the race unlikely, at the price of a syscall on every miss, even underRefreshMode::Neverwith nothing loading.What changes
The increment now happens before the claim, and is undone immediately if the claim finds nothing to load. With all accesses
SeqCst, every claimed slot is covered by an increment that precedes the claim, and the decrement only happens once loading is done (or failed). So a thread whose claim fails and which then reads the counter as zero knows that no claimed slot is still being loaded, and the yield in front of the wait loop can go. The wait loop itself stays as it was; theTODOabout a condition variable stays too.One side effect worth a glance: a thread on the miss path holds the counter at one for the few instructions between its increment, the failed claim and the decrement, so
collect_snapshot()and the wait loop can now spin briefly on a thread that is not loading anything. That window is a handful of atomic operations, so I did not try to avoid it.Measured
gix-odbtest binary, release profile, 200 000 lookups of ids that are not present against a fully loadedsmall-packs.gitfixture opened withrefresh_never(), five alternating runs of each binary on the same box:sched_yieldper missmain(
strace -cover 20 000 misses: 20 001sched_yieldbefore, none after. The onestatxper miss fromloose::Store::containsis untouched; the report names it separately and it is correct as is.) The yield is far more expensive on this machine than the 261 ns in the report; the syscall count is the part that should transfer.Checked
cargo test -p gix-odb: 70 passed; with--features parallel: 71 passed, andmulti_threaded_access_will_not_panicrun 30 times in a row without a failure.cargo clippy -p gix-odb --all-targets --features parallelreports nothing forgix-odbbeyond the workspace-wide "lint has been removed" note every crate gets on current stable;cargo fmt --all -- --checkis clean.mainwould not prove anything. If you would rather have a stress test anyway, say so and I will add one.🤖 Generated with Claude Code