Use generational identifiers for tracked structs #864

ibraheemdev · 2025-05-16T21:32:08Z

Pack a generation into input IDs.

The generation of a tracked struct is incremented after it is reused, allowing us to avoid read dependencies. Generational IDs were originally meant for #839, as adding the necessary read dependency on interned structs that may be reused introduced a large (~50%) regression to ty's incremental performance.

This increases the size of Id from a u32 to a u64. However, if the generation is restricted to u16, and ingredient indices are restricted to u16, this does not increase the size of DatabaseKeyIndex, so the memory usage effect is limited (~5% increase to ty's peak memory usage). However, this should allow us to implement garbage collection for interned values without significant performance concerns, so memory usage over time should benefit.

If the generation exceeds u16::MAX, we can fallback to adding read dependencies on tracked structs. An alternative would be to leak the slot, which would also allow us to remove the created_at field on tracked structs and may alleviate the memory usage concerns. This might be more feasible if the generation stole a few more bits from ingredient indices (as the number of ingredients is effectively static for a given salsa program).

This has a small (~4%) performance improvement on ty's benchmarks.

netlify · 2025-05-16T21:32:12Z

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	`5a2c276`
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/6830801652a6d30008710190

codspeed-hq · 2025-05-16T21:34:59Z

CodSpeed Performance Report

Merging #864 will degrade performances by 15.82%

_{Comparing ibraheemdev:ibraheem/gen-ids (5a2c276) with master (a12bf31)}

Summary

❌ 3 (👁 3) regressions
✅ 9 untouched benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
👁	`accumulator`	3.9 ms	4.4 ms	-12.1%
👁	`new[SupertypeInput]`	15.8 µs	16.4 µs	-4.09%
👁	`many_tracked_structs`	33.9 µs	40.2 µs	-15.82%

MichaReiser · 2025-05-17T14:15:44Z

This has a small (~4%) performance improvement on ty's benchmarks.

This is huge! I'm leaning towards removing the created_at field and leaking memos. It's still an improvement over what we have today and we can explore the right solution once we hit that limit (which could as well be that we increase the id size further)

davidbarsky

lgtm, merge whenever you'd like to?

MichaReiser · 2025-05-20T13:54:33Z

I'd be interested to explore the memory overhead if we changed DatabaseKeyIndex to store three separate u32 (ingredient, generation, and id) as that would, IMO, eliminate the need for a report_read fallback.

MichaReiser · 2025-05-20T16:54:04Z

tests/cycle_output.rs

@@ -152,6 +152,9 @@ fn revalidate_no_changes() {
            "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(400)) })",
            "salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })",
            "salsa_event(DidValidateMemoizedValue { database_key: query_d(Id(800)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(401)) })",


Huh, this is interesting

Do you know why this test changed?

I'm not sure. Maybe the memos are being validated instead of the function being re-executing, if the function previously had a read dependency on a tracked struct (and that dependency no longer exists)?

ibraheemdev · 2025-05-20T16:55:22Z

I updated the PR to store DatabaseKeyIndex as a u32 triple. This didn't seem to have a noticeable affect on memory usage, and allows us to remove the created_at field comfortably, which ends up being a net decrease compared to the previous version.

Unfortunately it looks like the benchmarks don't like this change...

src/tracked_struct.rs

MichaReiser · 2025-05-20T17:32:26Z

Is there any perf difference on ty? (When running on codspeed)

ibraheemdev · 2025-05-20T18:22:51Z

No performance difference on ty benchmarks (astral-sh/ruff#18226).

MichaReiser · 2025-05-20T20:47:07Z

I plan to review this tomorrow.

I'm okay with the regression given that it enables interned GC without a 50% incremental perf and memory regression. If you haven't done so already, maybe take an hour or two to see if you can spot the source of the regression in a local recorded profile (take the benchmark that regresses the most)

This is a substantial change where I'd like to get at least a thumbs up from r-a too, given that the performance is now regressing on micro benchmarks. Cc @Veykril

ibraheemdev · 2025-05-20T21:20:51Z

I'm okay with the regression given that it enables interned GC without a 50% incremental perf and memory regression. If you haven't done so already, maybe take an hour or two to see if you can spot the source of the regression in a local recorded profile (take the benchmark that regresses the most)

I don't think there is a specific source, I think the regression is directly related to the size of DatabaseKeyIndex increasing (e.g. worse cache utilization, and it specifically no longer fits into a standard register). It makes sense that the microbenchmarks are regressing, I'm not sure there's much we can do about it.

ibraheemdev · 2025-05-20T22:00:43Z

src/tracked_struct.rs

+                let updated_id = self.update(zalsa, current_revision, id, &current_deps, fields);
+                if id != updated_id {
+                    // Overwrite the previous ID if we are reusing the slot with new fields.
+                    zalsa_local.store_tracked_struct_id(identity, updated_id);


This feels a little wrong, but I think it is correct. If the struct is recreated with the same fields, we want to return the latest generation for the tracked structs to align with the memos. If it is recreated with new fields, we have to create a new generation and invalidate the previous version.

MichaReiser

This is awesome. I've a few smaller comments that should be addressed before landing (assuming r-a is on board with this change)

src/id.rs

src/key.rs

src/id.rs

src/tracked_struct.rs

MichaReiser · 2025-05-21T07:12:59Z

tests/cycle_output.rs

@@ -152,6 +152,9 @@ fn revalidate_no_changes() {
            "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(400)) })",
            "salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })",
            "salsa_event(DidValidateMemoizedValue { database_key: query_d(Id(800)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(401)) })",


Do you know why this test changed?

src/tracked_struct.rs

Veykril · 2025-05-22T07:43:21Z

I would like to cut another release before merging this if possible. I expect this to have a noticable memory impact for rust-analyzer, having this be part of a separate release cycle would make that easier to check.

MichaReiser · 2025-05-22T14:08:54Z

src/tracked_struct.rs

+            // the unlikely case that the ID is already at its maximum generation, we are forced to leak
+            // the previous slot and allocate a new value.
+            if id.generation() == u32::MAX {
+                return Err(());


I think it would be good to log at least an info message that we leaked this node.

ibraheemdev · 2025-05-22T14:11:17Z

When rebased onto this branch, #839 is actually a ~5% improvement to ty's incremental performance.

MichaReiser · 2025-05-23T09:42:48Z

@Veykril what's your timeline on the next release?

Veykril · 2025-05-23T10:33:27Z

I'm fine with cutting a release now if you want to land this to try it out in ruff/ty asap

MichaReiser · 2025-05-23T13:41:39Z

I'll land this once all feedback is addressed.

ibraheemdev force-pushed the ibraheem/gen-ids branch from 57ec532 to 66bbb88 Compare May 18, 2025 03:25

davidbarsky approved these changes May 20, 2025

View reviewed changes

ibraheemdev force-pushed the ibraheem/gen-ids branch from d980d6c to 5ce6735 Compare May 20, 2025 16:52

MichaReiser reviewed May 20, 2025

View reviewed changes

ibraheemdev commented May 20, 2025

View reviewed changes

src/tracked_struct.rs Outdated Show resolved Hide resolved

ibraheemdev force-pushed the ibraheem/gen-ids branch from 5ce6735 to 4920a8a Compare May 20, 2025 17:09

ibraheemdev added 2 commits May 20, 2025 13:42

use generational identifiers for tracked structs

81f3959

increase ID generations to 32-bits

e51adda

ibraheemdev force-pushed the ibraheem/gen-ids branch from 4920a8a to 19d00eb Compare May 20, 2025 17:42

remove created_at field from tracked structs

8b6cc9f

ibraheemdev force-pushed the ibraheem/gen-ids branch from 19d00eb to 8b6cc9f Compare May 20, 2025 21:58

ibraheemdev commented May 20, 2025

View reviewed changes

MichaReiser approved these changes May 21, 2025

View reviewed changes

ibraheemdev force-pushed the ibraheem/gen-ids branch from 6fbf77d to 9c27c3d Compare May 22, 2025 13:16

clean up tracked struct IDs to handle overflow

bc403d9

ibraheemdev force-pushed the ibraheem/gen-ids branch from 9c27c3d to bc403d9 Compare May 22, 2025 13:26

MichaReiser reviewed May 22, 2025

View reviewed changes

log tracing message for leaked tracked structs

5a2c276

ibraheemdev force-pushed the ibraheem/gen-ids branch from 3a47df9 to 5a2c276 Compare May 23, 2025 14:03

MichaReiser enabled auto-merge May 23, 2025 14:05

MichaReiser added this pull request to the merge queue May 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 23, 2025

MichaReiser added this pull request to the merge queue May 23, 2025

Merged via the queue into salsa-rs:master with commit f7b0856 May 23, 2025
11 checks passed

github-actions bot mentioned this pull request May 23, 2025

chore: release v0.23.0 #877

Open

ibraheemdev mentioned this pull request May 27, 2025

Simple LRU garbage collection for interned values #839

Open

Use generational identifiers for tracked structs #864

Use generational identifiers for tracked structs #864

Uh oh!

Conversation

ibraheemdev commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for salsa-rs canceled.

Uh oh!

codspeed-hq bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #864 will degrade performances by 15.82%

Summary

Benchmarks breakdown

Uh oh!

MichaReiser commented May 17, 2025

Uh oh!

davidbarsky left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented May 20, 2025

Uh oh!

MichaReiser May 20, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser May 21, 2025

Choose a reason for hiding this comment

Uh oh!

ibraheemdev May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ibraheemdev commented May 20, 2025

Uh oh!

Uh oh!

MichaReiser commented May 20, 2025

Uh oh!

ibraheemdev commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented May 20, 2025

Uh oh!

ibraheemdev commented May 20, 2025

Uh oh!

ibraheemdev May 20, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Veykril commented May 22, 2025

Uh oh!

MichaReiser May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ibraheemdev commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

Veykril commented May 23, 2025

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibraheemdev commented May 16, 2025 •

edited

Loading

netlify bot commented May 16, 2025 •

edited

Loading

codspeed-hq bot commented May 16, 2025 •

edited

Loading

ibraheemdev commented May 20, 2025 •

edited

Loading

ibraheemdev commented May 22, 2025 •

edited

Loading