Simple LRU garbage collection for interned values

[ibraheemdev]

Basic implementation of #597.

The garbage collection strategy used here is very simple.

• Slots that have not been accessed in the last 3 active revisions are always reused, and nothing else. Active revisions are those in which interned values were actually read to avoid counting empty revisions.
• Slots are never swept proactively, though this would be simple to add as an option.
• We also cannot collect low durability values, which would require &mut db. This could be supported with the above.

This is by no means a perfect strategy, but we can always tune it as needed once the infrastructure is in place. We may also want to expose the N active revisions constant as a configuration option in the #[interned] macro (it seems somewhat related to durability).

The LRU list is sharded by hash, very similar to dashmap. There seems to be no performance difference from the previous implementation in multi-threaded ty benchmarks (though there was a significant regression without sharding). There is also no significant performance difference in single-threaded benchmarks thanks to #864, which lets us avoid additional read dependencies.

My main concern is that this change adds 3 word-sized fields to interned values: an Id and two pointers for the intrusive LRU list. This seems unavoidable to implement garbage collection, but still something to note.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	286c167
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/683642844850650008c477c2

[codspeed-hq]

CodSpeed Performance Report

Merging #839 will not alter performance

_{Comparing ibraheemdev:interned-gc (286c167) with master (f9a9bb2)}

Summary

✅ 12 untouched benchmarks

[MichaReiser]

I'm seeing a lot of panics in red-knot, mostly Data was not interned in the latest revision for its durability. I'm not sure exactly why this is happening, but I suspect it's related to durabilities.

Interesting. I suspect this is new but I'm investigating a similar issue related to interned in cycles

[MichaReiser]

That makes me wonder if your PR might help to come up with simpler repros for the fixpoint issues that we're seeing. I should give this a try tomorrow.

On the perf side. You could try using the knot_benchmark script in the ruff repository

[MichaReiser]

I quickly tried the branch in Red Knot but I also got new panics that we didn't see before. That makes me suspect that some revision tracking might be off in this PR?

What I like to do to debug those issues is to compare the tracing output and see where things go different

[ibraheemdev]

I think the issue is unrelated to durabilities but about interned query arguments. If a query takes an interned struct as an argument then an older memo may be incorrectly verified after the slot has been reused, but the IDs still match. I think we have to check that the memo and first_interned_at revisions match to avoid this, but I'm not exactly sure where that would be.

[MichaReiser]

I think I understand. What you're saying is that query(interned) may be checked for changes in a later revision where interned has been collected.

I think will need something similar to what we have in tracked structs where salsa informs other ingredients that the memo was collected so that they can free resources as well if necessary:

https://github.com/MichaReiser/salsa/blob/953dc34ce5cfd7fca6ad040241c3cbebc11830e7/src/tracked_struct.rs#L597-L655

The second part in tracked struct that protects against this is that we separately track created_at and updated_at. maybe_changed_after can then compare created_at to see if this struct was recrated since rev_before.

[ibraheemdev]

I updated the PR to add read dependencies and clear memos on reuse (I believe we will need to defer reclamation of the memos), but there are a still some ty tests failing that I'm unsure about.

[ibraheemdev]

Hmm I'm not sure we can remove the read dependency on creation with generational IDs, we still need last_interned_at to be updated for any dependent queries. Maybe it would be cheaper to update that field directly when an interned value is read?

[MichaReiser]

Hmm I'm not sure we can remove the read dependency on creation with generational IDs, we still need last_interned_at to be updated for any dependent queries. Maybe it would be cheaper to update that field directly when an interned value is read?

No, I don't think we can remove it from the writing query. We need the dependency to ensure the interned value gets re-created when it was claimed previously

[ibraheemdev]

I updated the PR description with the new changes, this should be ready for review.