Skip to content

Conversation

MichaReiser
Copy link
Contributor

We currently store tracked structs twice:

  • In QueryRevisions::origin (as an output): This is mainly for historic reasons because tracked structs required to be verified (mark_validated_output), but that's no longer the case since we're using generational ids.
  • In QueryRevisionExtras::tracked_struct_ids: To get the tracked struct ID for a given tracked struct in a later revision (we want stable ids for better incrementality).

This PR removes tracked struct ids from origin. It requires tracking in the IdentityMap whether a tracked struct
was created in this revision (or if it's an entry from a previous revision but the tracked struct wasn't recreated)
so that we can remove now stale tracked structs in diff_outputs.

This should help reduce memory usage and potentially even improve performance because
it removes the active_query.add_output call for tracked structs and deep_verify_memo
no longer has to iterate over tracked structs.

Copy link

netlify bot commented Aug 9, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 6a63bff
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/689b0c1cef4f6e0008bd032b

Copy link

codspeed-hq bot commented Aug 9, 2025

CodSpeed Performance Report

Merging #969 will improve performances by ×3.4

Comparing MichaReiser:store-tracked-struct-ids-only-once (6a63bff) with master (a2cd1b8)

Summary

⚡ 1 improvements
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
many_tracked_structs 31.8 µs 9.2 µs ×3.4

@MichaReiser
Copy link
Contributor Author

This looks promising :)

@MichaReiser MichaReiser changed the title refactor: Remove tracked structs from outputs refactor: Remove tracked structs from querz outputs Aug 9, 2025
@MichaReiser MichaReiser changed the title refactor: Remove tracked structs from querz outputs refactor: Remove tracked structs from query outputs Aug 9, 2025
@MichaReiser MichaReiser requested a review from ibraheemdev August 9, 2025 16:24
@MichaReiser
Copy link
Contributor Author

The results on ty's benchmarks aren't as impressive but still a solid 1-3% performance win and a reduction in memory usage.

@ibraheemdev ibraheemdev force-pushed the store-tracked-struct-ids-only-once branch from 309015a to 803dd10 Compare August 11, 2025 18:56
@davidbarsky
Copy link
Contributor

The memory usage, for posterity: astral-sh/ruff#19843 (comment)

@ibraheemdev ibraheemdev added this pull request to the merge queue Aug 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 11, 2025
@MichaReiser MichaReiser force-pushed the store-tracked-struct-ids-only-once branch from 803dd10 to 53be770 Compare August 12, 2025 06:44
@MichaReiser
Copy link
Contributor Author

Uhhm, why?

@MichaReiser MichaReiser reopened this Aug 12, 2025
@MichaReiser
Copy link
Contributor Author

I don't get it. It seems that now there's a huge regression after I rebased past the persistence changes but they shouldn't matter?

@MichaReiser MichaReiser force-pushed the store-tracked-struct-ids-only-once branch from d082632 to b9d7340 Compare August 12, 2025 07:24
@MichaReiser
Copy link
Contributor Author

I can't reproduce this regression locally or in ty. I'm not sure what's up here with codspeed I'm sure something changed but it isn't clear what and I wonder if it's just because it now sees the LocalKey frame. I also noticed that the numbers differ by a lot if I make a change which the following commit undoes again.

@MichaReiser MichaReiser force-pushed the store-tracked-struct-ids-only-once branch from c388092 to c7b2d70 Compare August 12, 2025 09:28
@MichaReiser MichaReiser force-pushed the store-tracked-struct-ids-only-once branch from c7b2d70 to 6a63bff Compare August 12, 2025 09:40
@MichaReiser MichaReiser enabled auto-merge August 12, 2025 09:45
@MichaReiser MichaReiser added this pull request to the merge queue Aug 12, 2025
Merged via the queue into salsa-rs:master with commit e5bd9eb Aug 12, 2025
12 checks passed
@MichaReiser MichaReiser deleted the store-tracked-struct-ids-only-once branch August 12, 2025 09:57
@MichaReiser
Copy link
Contributor Author

Hmm, I think this introduced a subtle regression regarding fixpoint iteration.

Before: Salsa seeded the tracked_struct_ids in every iteration with the values from the previous revision and the values from the previous iteration.

Now: Salsa seeds the tracked_struct_ids initially with the values from the previous iteration. Later iterations seed tracked_struct_ids with the values from the previous iteration but any tracked struct id that wasn't created in the first iteration isn't copied over anymore because the new QueryRevisions removes unused tracked struct ids eagerly and not only when diff_outputs is called (which we only ran between the memo from the last revision with the memo from the new revision).

Now, this is sort of a pre-existing issue for queries that aren't cycle heads because they already had the behavior that we now observe for cycle heads too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants