Skip to content

Conversation

@MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Aug 14, 2025

This PR fixes a regression that I introduced with #969

#969 moves the tracked structs out from QueryRevisions::origin and instead tracks the "alive" tracked ids directly in IdentityMap. Completing the IdentityMap then returns to vecs:

  1. The alive tracked struct ids and their identity
  2. The not re-created tracked struct ids

Unlike before #969, the not re-created tracked struct ids are identified eagerly when completing the query. This isn't an issue for non-cyclic queries but it resulted in leaking if a cyclic query didn't create all tracked structs in the first iteration of the second revision.

This is fixed in this PR. I added a regression test and verified that the logs match the behavior before we merged #969.

@netlify
Copy link

netlify bot commented Aug 14, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 8b7e3b9
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/68a452aac192d700090c8928

@MichaReiser MichaReiser marked this pull request as draft August 14, 2025 18:43
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 14, 2025

CodSpeed Performance Report

Merging #979 will improve performances by 5.17%

Comparing MichaReiser:diff-tracked-structs-cycle (8b7e3b9) with master (411f844)

Summary

⚡ 2 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[Input] 2.4 µs 2.3 µs +5.17%
amortized[SupertypeInput] 3 µs 2.9 µs +5.11%

@MichaReiser MichaReiser reopened this Aug 14, 2025
@MichaReiser MichaReiser changed the title Fix tracked structs diffing in cycles fix: Delete not re-created tracked structs after fixpoint iteration Aug 15, 2025
@MichaReiser MichaReiser requested a review from carljm August 15, 2025 17:12
@MichaReiser MichaReiser marked this pull request as ready for review August 15, 2025 17:12
@MichaReiser MichaReiser added the bug Something isn't working label Aug 16, 2025
@MichaReiser
Copy link
Contributor Author

@carljm Could you take a look at this?

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.

Sorry, not sure how I missed this. Looks good, thank you! Needs a clippy expect in the test.

@MichaReiser MichaReiser force-pushed the diff-tracked-structs-cycle branch from 8cdb20e to 8b7e3b9 Compare August 19, 2025 10:32
@MichaReiser MichaReiser reopened this Aug 19, 2025
@MichaReiser MichaReiser enabled auto-merge August 19, 2025 11:58
@MichaReiser MichaReiser added this pull request to the merge queue Aug 19, 2025
Merged via the queue into salsa-rs:master with commit 0656eca Aug 19, 2025
17 of 19 checks passed
@MichaReiser MichaReiser deleted the diff-tracked-structs-cycle branch August 19, 2025 12:08
@github-actions github-actions bot mentioned this pull request Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants