Skip to content

Do not re-verify already verified memoized value in cycle verification #851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 8, 2025
Merged

Do not re-verify already verified memoized value in cycle verification #851

merged 1 commit into from
May 8, 2025

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented May 8, 2025 •
edited
Loading

Stacked on top of #852

@netlify Netlify
Copy link

netlify bot commented May 8, 2025 •
edited
Loading

✅ Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit ef93d38
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/681c7044098caa0008c6663c

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented May 8, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #851 will not alter performance

Comparing Veykril:veykril/push-ytnrltsrkurt (ef93d38) with master (00dbf01)

Summary

✅ 12 untouched benchmarks

@Veykril
Copy link
Member Author

Veykril commented May 8, 2025 •
edited
Loading

Seems like this has a slight overall green perf implication #852 did

Veykril
Veykril commented May 8, 2025
View reviewed changes
MichaReiser
MichaReiser reviewed May 8, 2025
View reviewed changes
src/function/maybe_changed_after.rs
if heads_non_empty {
// case 2 / 4
break 'cycle VerifyResult::Unchanged(inputs);
} else if !first_iteration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the elif !first_iteration a semantic change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is what causes the test changes you annotated as well below. I made that commit separately precisely to discuss (forgot to make a self review to start that discussion sorry). The change was made given the comment above declaring the 4 cases. Judging by case 3's description (the only one that actually continues the loop, we want to iterate it once more but we always re-add ourselves as the cylce head on this second iteration, meaning we will re-validate us once more (that is the cause for the duplicate DidValidateMemoizedValue events) in the second iteration and then continue again for a third time before finally bailing out.

Given the wording of the comment I interpreted this as that we want to merely run this a second time but no more. This is probably something that @carljm should have a look at.

I can split this PR into its two commits if the first one looks good as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty big speed up on our benchmarks https://codspeed.io/astral-sh/ruff/branches/micha%2Ffixpoint-changes

It would be nice if the code reflected the 1...4 ordering a little more. Especially if 1. could be first but this is a nit (which should also be the most likely branch taken at runtime)

MichaReiser
MichaReiser reviewed May 8, 2025
View reviewed changes
tests/cycle.rs
@@ -882,7 +882,6 @@ fn cycle_unchanged() {
[
"salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
"salsa_event(DidValidateMemoizedValue { database_key: min_panic(Id(2)) })",
"salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we see behavior changes here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removing a redundant verification (min_iterate(1) was already verified above), which is the point of the change as I understand it

MichaReiser
MichaReiser reviewed May 8, 2025
View reviewed changes
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the &mut CycleHead refactor. I think it helps clarify some things

@Veykril Veykril force-pushed the veykril/push-ytnrltsrkurt branch from 735cd15 to aa0ef5a Compare May 8, 2025 08:20
@Veykril Veykril changed the title Pass cycle heads as out parameter for maybe_changed_after Do not re-verify already verified memoized value in cycle verification May 8, 2025
@Veykril Veykril force-pushed the veykril/push-ytnrltsrkurt branch from aa0ef5a to bb8dc08 Compare May 8, 2025 08:22
@Veykril
Copy link
Member Author

Veykril commented May 8, 2025

Split out the first commit into #852 to leave the question of interest here

@Veykril Veykril force-pushed the veykril/push-ytnrltsrkurt branch from bb8dc08 to ef93d38 Compare May 8, 2025 08:50
MichaReiser
MichaReiser approved these changes May 8, 2025
View reviewed changes
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to and I verified that this doesn't introduce any new hangs but I'd prefer if Carl could take a look at this as well

carljm
carljm approved these changes May 8, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! This looks great to me.

tests/cycle.rs
@@ -882,7 +882,6 @@ fn cycle_unchanged() {
[
"salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
"salsa_event(DidValidateMemoizedValue { database_key: min_panic(Id(2)) })",
"salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removing a redundant verification (min_iterate(1) was already verified above), which is the point of the change as I understand it

@Veykril Veykril added this pull request to the merge queue May 8, 2025
@MichaReiser
Copy link
Contributor

Unrelated to your PR but one thing I and carl discussed is that we should verify that its guarantee that we exit the loop after N iterations and don't loop forever (hang).

Merged via the queue into salsa-rs:master with commit af69cc1 May 8, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-ytnrltsrkurt branch May 8, 2025 16:44
@github-actions github-actions bot mentioned this pull request May 8, 2025
Merged
MichaReiser
MichaReiser reviewed May 8, 2025
View reviewed changes
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this fix the r-a hang?

@MichaReiser MichaReiser mentioned this pull request May 8, 2025
Merged
@Veykril
Copy link
Member Author

Veykril commented May 9, 2025

No it did not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@carljm carljm carljm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Veykril @MichaReiser @carljm