Enable Garbage Collection for Interned Values #602

ibraheemdev · 2024-10-23T20:56:19Z

Per the mentoring instructions in #597.

Still needs some tests. Note this does not actually implement garbage collection but adds the necessary plumbing to do so.

netlify · 2024-10-23T20:56:35Z

✅ Deploy Preview for salsa-rs ready!

Name	Link
🔨 Latest commit	`5f17266`
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/67bffab3f183550008a0ef75
😎 Deploy Preview	https://deploy-preview-602--salsa-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codspeed-hq · 2024-10-23T20:58:34Z

CodSpeed Performance Report

Merging #602 will degrade performances by 22.49%

_{Comparing ibraheemdev:gc (5f17266) with master (99be5d9)}

Summary

❌ 1 regressions
✅ 10 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`accumulator`	3.8 ms	4.9 ms	-22.49%

MichaReiser

Nice. This looks good to me after adding a few tests.

src/function/specify.rs

src/interned.rs

nikomatsakis

Yes, looks good to me!

nikomatsakis · 2024-10-24T19:29:49Z

I see there is a test failure....have you investigated it at all?

nikomatsakis · 2024-10-24T19:43:09Z

My hunch is that the test is testing behavior that's not relevant anymore but I can look.

ibraheemdev · 2024-11-04T08:33:10Z

@nikomatsakis I'm also hitting the assertion error in this example:

#[salsa::tracked]
fn function<'db>(db: &'db dyn Database, input: Input) -> Interned<'db> {
    Interned::new(db, 0)
}

let input = Input::new(&db, 0);
function(&db, input);
input.set_field1(&mut db).to(1);
function(&db, input);

The function does not read the input so the interned value is memoized, meaning it is not re-interned in R2. What should be done in this case (and the similar failing test)?

I'm also a little unclear about the testing instructions. Should I add a log event for when values are re-interned, or is there another way I can test that? I'm also not sure how to access the reset method that I added in tests.

MichaReiser · 2024-11-04T15:57:14Z

I'm also a little unclear about the testing instructions. Should I add a log event for when values are re-interned, or is there another way I can test that?

That would sound reasonable to me, similar to the backdate event. We could also consider adding an event when a value is garbage collected.

MichaReiser · 2024-11-04T17:25:10Z

If I understand it correctly the problem is that interned ingredients created outside of a query are never revalidated because there's no corresponding query that creates them.

I see two options:

We forbid creating interned values outside of queries. That would match the behavior with tracked structs. The downside is that it technically becomes impossible to call a root salsa query with more than one argument because multi-argument functions intern the arguments (outside the query).
We disable garbage collection for ingredients created outside of queries because salsa can't track their creation. Salsa has to consider them as always referenced. That's a bit of a footgun but probably fine?

MichaReiser · 2024-11-04T17:56:35Z

I took a closer look at test_leaked_inputs_ignored and this is a slightly different variation of the above problem in the sense that the multi-argument-query is called as part of another query, but said query never re-executes and id_to_input panics.

I think the problem here is that function::maybe_changed_after never calls id_to_input when it short-circuits where it probably should, to at least mark the input still as used. However, it's unclear to me how that would work when the query using an interner is deeper in the tree (but the outermost query short-circuits, e.g. because the durability is higher)

nikomatsakis · 2024-11-06T15:39:10Z

We had a brief discussion in our meeting today--- @ibraheemdev I think values that are interned outside of any salsa query have to be concerned immortal.

src/interned.rs

ibraheemdev · 2025-01-12T01:52:49Z

The test_leaked_inputs_ignored test is now passing, but I added a test that still fails.

This one seems trickier. The query does not have a dependency on its input. However, changing the input still causes the database revision to be advanced. I'm not exactly sure why, but I believe the second query call only performs a shallow memoization check, meaning that maybe_changed_after is never called for the interned value, so it's revision is never updated, and the read panics.

ibraheemdev · 2025-01-12T01:57:59Z

src/interned.rs

+
+                // Record a dependency on this value.
+                let index = self.database_key_index(id);
+                zalsa_local.report_tracked_read(index, Durability::MAX, current_revision);


Just making sure, is current_revision the correct argument to pass here?

This feels incorrect, should it be the last_interned_at value?

MichaReiser · 2025-01-12T11:35:30Z

Oh this is interesting. I have to take a closer look. We probably have a similar problem for when the input has a higher durability and Salsa skips the deep verification because of it.

MichaReiser · 2025-01-13T09:06:27Z

Okay, I think I now understand the underlying problem. I'm not yet sure what the right fix is. I'll reach out in https://salsa.zulipchat.com/#narrow/channel/333573-Contributing-to-Salsa/topic/LRU.20.28GC.29.20for.20interned.20values.20.23597

MichaReiser · 2025-01-17T12:08:09Z

I thought more about this and I feel somewhat confident that we should store the Durability on the interned value, similar to what's done for tracked structs.

https://github.com/MichaReiser/salsa/blob/3e2d7f351894ec5b75df5bc1181cdf22b3243e95/src/tracked_struct.rs#L191-L196

What's different for interned values is that multiple queries can create the same tracked struct and the queries might also change their durability over time. For now, I'd suggest that we store the highest durability across all reads because we have to be pessimistic and assume the interned value only gets recreated after a HIGH durability change if we have one HIGH and two low durability queries that both create the same interned value.

The read path should compare against the most recent revision for the interned value's durability instead of using the current revision.

This should unblock this PR. What's unclear is how garbage collection would work because there's no guarantee that a query has to run in every revision. That means it's possible for interned values never to have been read in the last 1000 revisions, and they might get garbage collected because of it. However, the result of a cached query might still depend on the interned value (or even reference it in the output).

nikomatsakis · 2025-01-27T20:54:23Z

@MichaReiser @ibraheemdev -- just to confirm, this is still pending the changes we discussed in our last sync meeting, correct?

ibraheemdev · 2025-01-27T21:47:25Z

@nikomatsakis yes, I'll be working on it this week.

MichaReiser · 2025-02-19T17:58:55Z

Hmm, the accumulator regression is surprising. Did we accidentally break the optimization that skips traversing queries that are known not to have any accumulated values?

MichaReiser · 2025-02-19T18:14:28Z

I don't see anything obvious that should have caused the regression but I suspect it's simply the fact that we now need to look up the revision from the active query?

This otherwise looks good to me. Do we need to remove any code? I remember you mentioned that we don't need the "immortal" handling.

src/interned.rs

ibraheemdev · 2025-02-27T05:44:58Z

I'm going to try to add some more tests before this gets merged.

Veykril · 2025-02-28T07:20:39Z

src/interned.rs

+
+                id
+            }
+
            // We won any races so should intern the data
            Err(slot) => {
                let zalsa = db.zalsa();


nit, we already have zalsa in scope here

Suggested change

let zalsa = db.zalsa();

Veykril · 2025-02-28T07:21:43Z

src/interned.rs

        let internal_data = db.zalsa().table().get::<Value<C>>(id);
+        let last_changed_revision = db.zalsa().last_changed_revision(internal_data.durability());


Suggested change

let internal_data = db.zalsa().table().get::<Value<C>>(id);

let last_changed_revision = db.zalsa().last_changed_revision(internal_data.durability());

let zalsa = db.zalsa();

let internal_data = zalsa.table().get::<Value<C>>(id);

let last_changed_revision = zalsa.last_changed_revision(internal_data.durability());

to reduce the amount of dynamically dispatched calls

Veykril · 2025-02-28T07:22:41Z

src/interned.rs

+        // Trigger a new revision.
+        let _zalsa_mut = db.zalsa_mut();


Suggested change

// Trigger a new revision.

let _zalsa_mut = db.zalsa_mut();

// Trigger a new revision.

db.zalsa_mut().new_revision();

The behavior here has changed on master.

Veykril · 2025-03-02T12:04:25Z

src/key.rs

-pub struct OutputDependencyIndex {
+pub struct DatabaseKeyIndex {


What is the motivation behind discarding the type level differentiation of input and output keys btw?

I'd have to take a closer look but I think the reason is simply that they're the same now? see #597

ibraheemdev force-pushed the gc branch from 1331b99 to db71a89 Compare October 23, 2024 21:06

MichaReiser reviewed Oct 24, 2024

View reviewed changes

src/function/specify.rs Outdated Show resolved Hide resolved

src/interned.rs Outdated Show resolved Hide resolved

nikomatsakis approved these changes Oct 24, 2024

View reviewed changes

MichaReiser reviewed Nov 15, 2024

View reviewed changes

src/interned.rs Outdated Show resolved Hide resolved

ibraheemdev force-pushed the gc branch from c6077c4 to 6505453 Compare January 12, 2025 01:48

ibraheemdev commented Jan 12, 2025

View reviewed changes

ibraheemdev marked this pull request as draft January 27, 2025 21:47

ibraheemdev force-pushed the gc branch from 506a57c to ca187bf Compare February 18, 2025 23:43

ibraheemdev marked this pull request as ready for review February 18, 2025 23:44

ibraheemdev force-pushed the gc branch from ca187bf to 7bdf587 Compare February 19, 2025 00:19

nikomatsakis assigned MichaReiser Feb 19, 2025

ibraheemdev commented Feb 19, 2025

View reviewed changes

src/interned.rs Outdated Show resolved Hide resolved

src/interned.rs Show resolved Hide resolved

remove table-wide dependencies

0543473

ibraheemdev added 4 commits February 27, 2025 00:37

add plumbing to reuse interned slots

fbf51a9

record durabilities on interned values

63af587

appease clippy

d52ab16

remove immortal interned value logic

5f17266

ibraheemdev force-pushed the gc branch from 3e591c1 to 5f17266 Compare February 27, 2025 05:40

Veykril reviewed Feb 28, 2025

View reviewed changes

Veykril reviewed Mar 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Garbage Collection for Interned Values #602

Enable Garbage Collection for Interned Values #602

ibraheemdev commented Oct 23, 2024 •

edited

Loading

netlify bot commented Oct 23, 2024 •

edited

Loading

codspeed-hq bot commented Oct 23, 2024 •

edited

Loading

MichaReiser left a comment

nikomatsakis left a comment

nikomatsakis commented Oct 24, 2024

nikomatsakis commented Oct 24, 2024

ibraheemdev commented Nov 4, 2024 •

edited

Loading

MichaReiser commented Nov 4, 2024

MichaReiser commented Nov 4, 2024

MichaReiser commented Nov 4, 2024

nikomatsakis commented Nov 6, 2024

ibraheemdev commented Jan 12, 2025 •

edited

Loading

ibraheemdev Jan 12, 2025

ibraheemdev Feb 27, 2025

MichaReiser commented Jan 12, 2025

MichaReiser commented Jan 13, 2025

MichaReiser commented Jan 17, 2025

nikomatsakis commented Jan 27, 2025

ibraheemdev commented Jan 27, 2025

MichaReiser commented Feb 19, 2025

MichaReiser commented Feb 19, 2025

ibraheemdev commented Feb 27, 2025

Veykril Feb 28, 2025

Veykril Feb 28, 2025

Veykril Feb 28, 2025

Veykril Mar 2, 2025

MichaReiser Mar 2, 2025

		let internal_data = db.zalsa().table().get::<Value<C>>(id);
		let last_changed_revision = db.zalsa().last_changed_revision(internal_data.durability());

		pub struct OutputDependencyIndex {
		pub struct DatabaseKeyIndex {

Enable Garbage Collection for Interned Values #602

Are you sure you want to change the base?

Enable Garbage Collection for Interned Values #602

Conversation

ibraheemdev commented Oct 23, 2024 • edited Loading

netlify bot commented Oct 23, 2024 • edited Loading

✅ Deploy Preview for salsa-rs ready!

codspeed-hq bot commented Oct 23, 2024 • edited Loading

Merging #602 will degrade performances by 22.49%

Summary

Benchmarks breakdown

MichaReiser left a comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Oct 24, 2024

nikomatsakis commented Oct 24, 2024

ibraheemdev commented Nov 4, 2024 • edited Loading

MichaReiser commented Nov 4, 2024

MichaReiser commented Nov 4, 2024

MichaReiser commented Nov 4, 2024

nikomatsakis commented Nov 6, 2024

ibraheemdev commented Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Jan 12, 2025

MichaReiser commented Jan 13, 2025

MichaReiser commented Jan 17, 2025

nikomatsakis commented Jan 27, 2025

ibraheemdev commented Jan 27, 2025

MichaReiser commented Feb 19, 2025

MichaReiser commented Feb 19, 2025

ibraheemdev commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibraheemdev commented Oct 23, 2024 •

edited

Loading

netlify bot commented Oct 23, 2024 •

edited

Loading

codspeed-hq bot commented Oct 23, 2024 •

edited

Loading

ibraheemdev commented Nov 4, 2024 •

edited

Loading

ibraheemdev commented Jan 12, 2025 •

edited

Loading