Use generational identifiers for tracked structs

[ibraheemdev]

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]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	19d00eb
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/682cbf1f736e72000800ab23

[codspeed-hq]

CodSpeed Performance Report

Merging #864 will degrade performances by 15.98%

_{Comparing ibraheemdev:ibraheem/gen-ids (19d00eb) with master (2d4321e)}

Summary

❌ 5 regressions
✅ 7 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	accumulator	3.9 ms	4.4 ms	-12.01%
❌	amortized[Input]	3 µs	3.2 µs	-5.64%
❌	amortized[InternedInput]	2.9 µs	3.1 µs	-5.8%
❌	amortized[SupertypeInput]	3.6 µs	3.8 µs	-4.93%
❌	many_tracked_structs	33.8 µs	40.2 µs	-15.98%

[MichaReiser]

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]

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]

Huh, this is interesting

[ibraheemdev]

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...

[ibraheemdev]

I'm a little unsure about this but it seems correct?

[MichaReiser]

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

[ibraheemdev]

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

[MichaReiser]

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]

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.