Reduce memory usage by deduplicating type information

[ChayimFriedman2]

We were storing the type information, 3 words wide, for each memo in each slot, while it is always constant wrt. the ingredient (different slots of the same ingredients will always have the same memos in the same order). This introduces some more unsafety, and the result wasn't as fast so I also had to use some lock-free structures, but the result is worth it: this shaves off 230mb from rust-analyzer with new Salsa.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #649 will not alter performance

_{Comparing ChayimFriedman2:common-typeid-real (fd9cfb8) with master (7d417ac)}

Summary

✅ 11 untouched benchmarks

[nikomatsakis]

I love the idea, but I'm finding myself a bit confused-- we have some tech debt here in that the mdbook is not complete or up-to-date, I'm realizing that I wish it were, because I'd love to read an mdbook update explaining the data structure(s) here. It feels like we could be more efficient still, but I've probably just not fully brought everything back in cache

[ChayimFriedman2]

@nikomatsakis I addressed your comments.

[MichaReiser]

The benchmark regressions are a bit concerning (as high as 8%). Do we understand where they come from?

[Veykril]

I wonder, could we maybe store the types info in the corresponding Page instead? Then we only have duplication of type info among pages which is neglible while not having to go through the ingredient on lookups. It would also keep more info (for drop and the like) localized

[ChayimFriedman2]

Hmm... If we share it in an Arc it should be very little duplication.

[nikomatsakis]

I don't quite follow here--I'll try to make time tomorrow to review this PR. I'm still finding myself a bit confused about what type information exactly we are deduplicating and where.

[ChayimFriedman2]

@Veykril I implemented your suggestion to store the types on the page.

[ChayimFriedman2]

@MichaReiser I've investigated the perf regressions and I think they should be fixed now.

[MichaReiser]

Thanks. The regression now seems to be specific to benchmarks where we primarily create elements. Can we try to optimize that code path as well (a 18% regression is rather significant)

[ChayimFriedman2]

@MichaReiser I was referring to that regression, I think I fixed it.

[MichaReiser]

@ChayimFriedman2 Hmm, in that case I'm unsure if the underlying problem was addressed because the codspeed benchmark still show an 18% regression.

[ChayimFriedman2]

@MichaReiser It wasn't reran back then, now it shows an 11% regression. I'll see if I can shrink it even more.

[ChayimFriedman2]

I think the benchmarks are flawed. They re-create the ingredients on every iteration, which is very much not like real-world usages which create the ingredient once. I'll put a PR to fix this.

[ChayimFriedman2]

OK, after #667 it shows there is no perf regression.

[nikomatsakis]

@ChayimFriedman2 needs rebase

[ChayimFriedman2]

@nikomatsakis Rebased.

[Veykril]

Then why does this not just take &mut self

[ChayimFriedman2]

Problems with the borrow checker when calling Page::get() borrows the entire page then we cannot access Page::memo_types.

[Veykril]

This change seems unnecessary to me, we can just use get_mut and keep the memos_mut accessor as before

[ChayimFriedman2]

But we need to attach the types to it anyway. I can change create a new type MemoTableWithTypesMut, but this just seems unnecessary. And given it does not return a mutable reference anymore, memos_mut() seems inappropriate name now.

[Veykril]

I don't mind the name change, I was more raising this regarding the unnecessary unsafe block. There is no reason to decay the mutable references in this call chain to an immutable one, doing so results in this extra unsafe that isn't really necessary otherwise

[Veykril]

Ah, given your other comment to my review I guess that would require duplicating the function for mutability

[ChayimFriedman2]

What do you mean by

duplicating the function for mutability

?

[Veykril]

Problems with the borrow checker when calling Page::get() borrows the entire page then we cannot access Page::memo_types.

I interpreted this as you'd needing to keep the current version for the borrowck issue either way

[ChayimFriedman2]

No, what I meant is that if I this function takes &mut self, then Page.get_mut().memos_mut() clashes with Page.memo_types, leading to a borrowck error.

[MichaReiser]

OK, after #667 it shows there is no perf regression.

It seems the perf regression is back :(

[ChayimFriedman2]

@MichaReiser I rebased and now they're at 5% max. It's hard to investigate because codspeed doesn't identify the same methods, so I don't know what is the cause, but the 8% I'm pretty sure was measurement error or something like that (e.g. it showed a regression for shallow_verify_memo(), where literally nothing it calls changed).

[MichaReiser]

The benchmarks have been fairly stable after @Veykril made some changes. That's why I'm not convinced that they're outright wrong. Have you tried recording a profile locally and e.g. comparing them in firefox-profiler?

[Veykril]

There does seem to be some noise still left as my rebased #710 version reports fairly different perf changes now (though I don't think the PR here is entirely noise)

[MichaReiser]

There does seem to be some noise still left as my rebased #710 version reports fairly different perf changes now (though I don't think the PR here is entirely noise)

Rebasing does change the base to which codspeed reports the performance. So that makes sense to some extend. What's interesting is that the perf results are very stable for e.g. https://codspeed.io/salsa-rs/salsa/branches/Veykril%3Aveykril%2Fpush-orlktsrpwzrv

[ChayimFriedman2]

I'm trying to investigate the regressions locally now.

[ChayimFriedman2]

The benchmark regressions seem unavoidable (I also suspect part of them is noise), and they are not large. The main reason is having two different RwLocks to lock and unlock (only read lock though) when accessing a memo: the types and the memos, instead of just one. I can't see an easy way to get rid of this, so I think the memory decrease is worth the perf penalty (some ideas I had about how we can potentially mitigate the lock on the types: synchronizing on the memos lock instead, but that means we have to go through the entire database and lock each and every memo when we want to register an ingredient; not having a lock, and instead use an atomic pointer and replace it with the new types when registering an ingredient, either with an arc-swap or without even reference counting: arc-swap is doable, but the overhead is already pretty tiny and I don't know how much it'll save, and custom-made implementation will mean we need some kind of GC to collect the unreachable types on new revision - we already have one for memos, but expanding it to types won't be trivial; or even statically register the memo ingredients at compile-time with something like linkme - while I look the idea, it will require throughout design work).