fix(thread_aware): dedup factory call on first relocate when source =…#399
Draft
fix(thread_aware): dedup factory call on first relocate when source =…#399
Conversation
…= destination Arc<T, S>::with_closure eagerly builds self.value for the calling thread's affinity but does not record which affinity it was. The first `relocated(source, destination)` call observes `source` from the caller — by contract the affinity of the thread that ran `with_closure`. When that first relocate has `source == destination`, the eager `self.value` is already the correct per-affinity value, so reuse it instead of running the factory again. Without this shortcut, `scheduler.spawn_multiple(Instantiation::All, ...)` called from a worker thread races on the storage write lock: the `source==destination` task can win first and factory-build (with no source-seed step, since the existing `source != destination` guard elides it) before any peer task populates the source slot via the end-of-function fallback. The result is one extra factory invocation non-deterministically, observable as `counts > len` in `fetch::isolated_on_oxidizer` (e.g. `9 > 8`). This complements PR #310 (which fixed storage corruption from the `source == destination` path) by also eliminating the extra factory call on that same path. Replaces the previous `test_relocated_source_equals_destination_does_not_corrupt_storage` regression test, which was asserting the now-fixed behavior (factory re-run on first `relocate(s, s)`). Adds a deterministic `Arc::ptr_eq` test covering the new invariant. AB#7312281 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #399 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 225 225
Lines 16350 16355 +5
=======================================
+ Hits 16350 16355 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
…= destination
Arc<T, S>::with_closure eagerly builds self.value for the calling thread's affinity but does not record which affinity it was. The first
relocated(source, destination)call observessourcefrom the caller — by contract the affinity of the thread that ranwith_closure.When that first relocate has
source == destination, the eagerself.valueis already the correct per-affinity value, so reuse it instead of running the factory again.Without this shortcut,
scheduler.spawn_multiple(Instantiation::All, ...)called from a worker thread races on the storage write lock: thesource==destinationtask can win first and factory-build (with no source-seed step, since the existingsource != destinationguard elides it) before any peer task populates the source slot via the end-of-function fallback. The result is one extra factory invocation non-deterministically, observable ascounts > leninfetch::isolated_on_oxidizer(e.g.9 > 8).This complements PR #310 (which fixed storage corruption from the
source == destinationpath) by also eliminating the extra factory call on that same path.Replaces the previous
test_relocated_source_equals_destination_does_not_corrupt_storageregression test, which was asserting the now-fixed behavior (factory re-run on firstrelocate(s, s)). Adds a deterministicArc::ptr_eqtest covering the new invariant.AB#7312281