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.
|
src/function/maybe_changed_after.rs
Outdated
| 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?
There was a problem hiding this comment.
Hmm, I gave this a quick try with building a test but couldn't really figure out one that hits this. I don't really know the accumulation stuff well enough here
CodSpeed Performance ReportMerging #780 will improve performances by 4.71%Comparing Summary
Benchmarks breakdown
|
Veykril
force-pushed
the
veykril/push-zxssxpkmuqlr
branch
from
a552fd9 to
64b0746
Compare
src/function/maybe_changed_after.rs
Outdated
| 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
src/zalsa_local.rs
Outdated
| // 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.
There was a problem hiding this comment.
Not changing it isn't really an option though, as we then are back to eagerly loading the atomic. Unless we pass the atomic field by ref I suppose
There was a problem hiding this comment.
Went for that and #[inline]'d the function given ti has only one use site
Veykril
force-pushed
the
veykril/push-zxssxpkmuqlr
branch
from
e046789 to
a45711a
Compare
3 participants
No description provided.