feature: add #[salsa::supertype]

[davidbarsky]

This PR implements #578 and is (currently) stacked atop of #676. All authorship/credit for this work goes to @ChayimFriedman2 for the implementation, I largely renamed some stuff.

Two notes on this PR:

1. There's no corresponding macro_rules! macro for #[salsa::supertype]. We can add it, need be.
2. I've commented out some hand-written PartialEq implementations because they caused a bunch of test failures in rust-analyzer. Those test failures stemmed from rust-analyzer's native, non-Salsa powered interner, which persists even after we changed databases in tests! For more details, see what Chayim wrote on Zulip. Instead of commenting things out, I'll make these options on the attribute macro.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	391af83
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/67bb3864df40d1000858b240

[codspeed-hq]

CodSpeed Performance Report

Merging #677 will not alter performance

_{Comparing davidbarsky:davidbarsky/supertypes (d7bf505) with master (198db98)}

Summary

✅ 9 untouched benchmarks

[Veykril]

Of note is that this implements a different design of #578, this design here having the benefit of retaining the enum being an enum (where as the issue would've transformed it into just another id that you'd need to access variants via methods).

[Veykril]

I've commented out some hand-written PartialEq implementations because they caused a bunch of test failures in rust-analyzer. Those test failures stemmed from rust-analyzer's native, non-Salsa powered interner, which persists even after we changed databases in tests! For more details, see what Chayim wrote on Zulip. Instead of commenting things out, I'll make these options on the attribute macro.

Hmm that sounds problematic, I don't see how adding an option to the macro fixes this? Are you saying we'd want to opt out of those impls in rust-analyzer until we replaced the type interner with salsa (by using the new trait solver)?

[davidbarsky]

Are you saying we'd want to opt out of those impls in rust-analyzer until we replaced the type interner with salsa (by using the new trait solver)?

Yes. In the meantime, Jack has also asked us to avoid making changes to the interner, as his changes depend on it being in place until Salsa's interned structs can natively handle/represent a TyKind.

[Veykril]

Are you saying we'd want to opt out of those impls in rust-analyzer until we replaced the type interner with salsa (by using the new trait solver)?

Yes. In the meantime, Jack has also asked us to avoid making changes to the interner, as his changes depend on it being in place until Salsa's interned structs can natively handle/represent a TyKind.

Understandable but won't this have effects on the incrementality of rust-analyzer?

[Veykril]

Propopsing a different store scheme davidbarsky#1 that wastes a tiny bit of memory for faster IngredientIndex to MemoIngredientIndex mapping. It does introduce pointer chasing but I don't think that will be problematic in comparison. only for the enum case.

Edit: It also skips the mapping entirely now for structs, so you only pay perf / memory usage for enums you use now.

[davidbarsky]

I don't think so, or at the very least, I haven't noticed any issues while running rust-analyzer under new Salsa, as I have been for the past month.

For what it's worth, I think these test failures go away if we run the test suite under nextest, as we don't really create new database instances under normal IDE conditions.

[davidbarsky]

Okay, I think this is ready for review. I've addressed Lukas's comments, but I've decided to punt on the derive/equality stuff until later—I'd have to rewrite this derive as an attribute macro and I don't really want to do that right now. I've opened #694 to track this.

[MichaReiser]

I haven't followed the discussion or reviewed the PR. So maybe this is something that has come up or has been discussed before but do we understand why this decreases performance by as much as 10%? I'm a bit surprised by this because none of the existing benchmarks use this feature, right?

[davidbarsky]

So maybe this is something that has come up or has been discussed before but do we understand why this decreases performance by as much as 10%? I'm a bit surprised by this because none of the existing benchmarks use this feature, right?

I'm a bit surprised as well. The most recent runs (16 hours ago, as of me writing this comment) shows that 10% regression on amortized[InternedInput], whereas older runs from 11 days ago show a far more minor 1% decrease in performance on the same benchmark. However, the changes from 12 days ago show a 6% decrease in performance.

This is a somewhat long-winded of saying "no, not really", as it wasn't really on my radar until you pointed it out. I don't think those regression are pure noise, especially since we've added src/memo_ingredient_indices.rs and introduced a bit of indirection with the FromIdWithDb, where the latter now requires callers to pass in a database alongside the ID.

[davidbarsky]

Frustratingly, I can't seem to get all changes the that I've force-pushed #684, but I recall rebasing last week over #673 and #684. I wonder if the changes introduced by those soundness fixes interacted suboptimally with the changes in this PR: I remember thinking a bunch as to how to resolve those conflicts appropriately.

[davidbarsky]

If I were to guess—and again, this is a guess that I haven't really dug into properly—the changes to reset_for_new_revision in #684 and PR resulted in more substantial regression than I otherwise expected.

[MichaReiser]

If I were to guess—and again, this is a guess that I haven't really dug into properly—the changes to reset_for_new_revision in #684 and PR resulted in more substantial regression than I otherwise expected.

You mean that specific PR or that your PR is now slower because it doesn't play well with the changes made in #684?

[davidbarsky]

If I were to guess—and again, this is a guess that I haven't really dug into properly—the changes to reset_for_new_revision in #684 and PR resulted in more substantial regression than I otherwise expected.

You mean that specific PR or that your PR is now slower because it doesn't play well with the changes made in #684?

The latter. Alone, the changes seem fine, but together, we get a ~10% regression. I don't think we can or should revert #684, I just think the current state is fixable bad luck.

[ChayimFriedman2]

If I had to guess, the perf regressions are from here, because of calling zalsa() on dyn Database twice (we call it in fetch_hot() too), and from Zalsa::ingredient_index(), because we call the dyn Page to retrieve the ingredient.

If I'm right, it is possible to fix them, by giving the zalsa we use here to fetch_hot(), and batching the page calls, by delaying memo_ingredient_index(), and instead adding a Page::ingredient_index_and_memos() method that combines Page::memos() and Page::ingredient_index(), and call it from get_memo_from_table_for().

[davidbarsky]

@ChayimFriedman2 If I understand you correctly, you're saying that there are two things that we can do:

1. Batch up calls to zalsa in src/function/fetch.rs. I did this locally and this helped marginally in my local benchmarks, but it could also just be noise.
2. Combine calls to ingredient_index until the latest possible point. I'm having difficulty understanding this part.

On the second point, it seems like the only case where this would be useful is in src/function.rs' IngredientImpl::reset_for_new_revision:

fn reset_for_new_revision(&mut self, table: &mut Table) {
    self.lru.for_each_evicted(|evict| {
        let ingredient_index = table.ingredient_index(evict);
        self.evict_value_from_memo_for(table.memos_mut(evict), ingredient_index);
    });
    std::mem::take(&mut self.deleted_entries);
}

[davidbarsky]

Oh, I see what you mean now. For whatever reason, I was searching for calls to ingredient_index and memo_table_for instead of get_memo_from_table_for. My bad.

[Veykril]

We actually unnecessarily call zalsa here for non supertype indices (its unused in that path) so we can be lazy here. I'll fix that up as a follow up to this PR though. Imo we should rebase this now and then merge it to make progress.

[davidbarsky]

Sounds good. That would get things closer to an alpha.1 release and gets us closer to unblocking @carljm from landing his fixpoint iteration changes.

@MichaReiser Are you okay with us landing this and fixing the regressions after (within, say, a day)?

[Veykril]

I rebased this PR in #727 and added the proposed change, no regressions anymore

[Veykril]

regressions are down to 5% worst now, I assume you merely rebased?

[davidbarsky]

regressions are down to 5% worst now, I assume you merely rebased?

Yeah, I just rebased, which notably captured the boxcar changes.

[Veykril]

Opened a PR to your branch to add a small benchmark that exerts the supertype stuff davidbarsky#3

[davidbarsky]

Closing in favor of #727.