refactor: Discard unnecessary atomic load #780
Conversation
| db, | ||
| revision_now, | ||
| database_key_index, | ||
| memo.revisions.accumulated_inputs.load(), |
There was a problem hiding this comment.
This one specifically, we load the value here just to set it to the same one
✅ Deploy Preview for salsa-rs canceled.
|
| break 'cycle VerifyResult::Unchanged( | ||
| InputAccumulatedValues::Empty, | ||
| CycleHeads::from(cycle_heads), | ||
| ); | ||
| break 'cycle VerifyResult::Unchanged(inputs, CycleHeads::from(cycle_heads)); |
There was a problem hiding this comment.
I'd like to get a check on this, I believe Empty by default is incorrect here hence the change, but I am not 100% sure
There was a problem hiding this comment.
Oh, sorry for missing this ping, apparently twice! I think this is change is right. It would probably be ideal to write a test that fails without this fix, though. If this fix is needed, I don't think the test should even require a cycle, as this is also the path for non-cycles. It would probably be a test with accumulated values across multiple revisions, with some inputs changing but some sub-graph (that has accumulated values in it) not changing?
CodSpeed Performance ReportMerging #780 will not alter performanceComparing Summary
|
Veykril
force-pushed
the
veykril/push-zxssxpkmuqlr
branch
from
a552fd9 to
64b0746
Compare
| break 'cycle VerifyResult::Unchanged( | ||
| InputAccumulatedValues::Empty, | ||
| CycleHeads::from(cycle_heads), | ||
| ); | ||
| break 'cycle VerifyResult::Unchanged(inputs, CycleHeads::from(cycle_heads)); |
Unsure why I can't reply to that but right, forgot to call that change out. It looks like a bug to me but please double check |
Veykril
force-pushed
the
veykril/push-zxssxpkmuqlr
branch
from
64b0746 to
e046789
Compare
| // We don't access the accumulator anyways | ||
| let revisions = AssertUnwindSafe(revisions); | ||
| self.with_query_stack(|stack| { | ||
| if let Some(top_query) = stack.last_mut() { | ||
| top_query.add_read(input, durability, changed_at, accumulated, cycle_heads); | ||
| top_query.add_read(input, { revisions }.0, cycle_heads); | ||
| } | ||
| }) |
There was a problem hiding this comment.
I think I prefer the old method signature in that case to avoid this very easy to get outdated comment about accumulators.
Not changing the signature (or only remuving accumulated) also has the benefit that it makes the diff a bit clearer.
| @@ -217,12 +216,12 @@ impl<V> Memo<V> { | |||
|
|
|||
| /// Mark memo as having been verified in the `revision_now`, which should | |||
| /// be the current revision. | |||
| #[inline] | |||
There was a problem hiding this comment.
It would be great to have a comment here mentioning that it's the caller's responsibility to update accumulated if their accumulated values have changed since the query ran last
No description provided.