Repository navigation
Fix order-dependent complexity_n accumulation across threads - #2382
Merged
Merged
Conversation
…hmarks. Failure path: In a `->Threads(N)->Complexity()` benchmark where one thread calls `SetComplexityN()` and another does not (e.g., only `thread_index() == 0` sets it), `results.complexity_n` starts unset in src/thread_manager.h:31; if a setting thread acquires `manager->GetBenchmarkMutex()` before a non-setting thread, we clear `results.complexity_n` back to unset, whereas if the non-setting thread acquires the mutex first, the setting thread overwrites it with a valid value. Checked: `RunThreads` runs threads concurrently without ordering mutex acquisition, and `ThreadManager::Result` has no separate field distinguishing "no thread reported yet" from "a thread reported without setting complexity_n". Fix: Track whether any thread has reported (or whether any thread missed `SetComplexityN()`) with a dedicated flag in `ThreadManager::Result` instead of overloading `!results.complexity_n.has_value()`, so all thread completion orders produce a deterministic result.
Collaborator
|
Thank you! |
Member
Author
|
we could make it an error not to report i guess... i thought this defensive approach was safer (and it's a regression from the optional thing rather than a change in prior behaviour). |
Collaborator
I kind-of expected such subtle invariants to be exposed by that change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since #2346 made complexity_n an OptionalComplexityN, RunInThread used
"results.complexity_n is unset" to mean both "no thread has reported
yet" and "a thread reported without calling SetComplexityN()". In a
->Threads(N)->Complexity() benchmark where only some threads call
SetComplexityN() (e.g. only thread_index() == 0), the result depended
on which thread took the benchmark mutex first:
to unset, so ComputeBigO aborted in debug builds ("Did you forget to
call SetComplexityN?") or used -1 in release builds.
value.
Now threads that did not call SetComplexityN() leave the accumulated
value alone, and threads that did either start it or add to it. The
result is unset only if no thread set it, and it no longer depends on
thread completion order. This matches the behaviour from before #2346,
where complexity_n defaulted to 0 and was summed across threads.
Add a test with a two-thread Complexity() benchmark where only thread 0
calls SetComplexityN(). The other thread sleeps after its loop, so
thread 0 always reports first. Without the fix this test aborts.
AI disclosure: this change was developed with the help of an AI coding
assistant (Jetski/Gemini). I reviewed and verified the change.