refactor: Per ingredient sync table

[Veykril]

I believe given that the lock is held fairly shortly we ought to be fine with this coarser grained locking. A lock per ingredient to claim a sync which means we only content for the lock when two queries try to access something from the same ingredient. This reduces memory usage by a good chunk.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #650 will degrade performances by 4.96%

_{Comparing Veykril:veykril/push-rxpmtpukknru (d3d3d65) with master (20a347e)}

Summary

❌ 4 regressions
✅ 8 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	amortized[Input]	3.4 µs	3.5 µs	-4.96%
❌	mutating[10]	12.9 µs	13.5 µs	-4.85%
❌	mutating[20]	13 µs	13.7 µs	-4.79%
❌	mutating[30]	13.2 µs	13.8 µs	-4.74%

[davidbarsky]

I rebased this change atop of a few others (#649 and Chayim's enum changes), and this change reduced rust-analyzer's analysis-stats memory usage by 240 megabytes: overall memory usage is down to 3360mb, down from an initial 8 gigabytes!

However, I'm unsure of of the impact of this when it comes to parallelism, so I don't feel comfortable stamping this PR.

[ChayimFriedman2]

@davidbarsky As an alternative I have my branch (unpublished), that shrinks SyncState from 16 to 2 bytes, without hurting parallelism. I don't remember how much it saves compared to this branch, but I can check.

[Veykril]

I think putting up your changes for discussion would be good in general (as mine do change semantics wrt to parallelism)

[davidbarsky]

Lukas, I assume you're replying to Chayim, but here are the branches I have right now:

• rust-analyzer on new Salsa: rust-lang/rust-analyzer@master...davidbarsky:rust-analyzer:davidbarsky/port-rust-analyzer-to-new-salsa
• All the Salsa changes used by rust-analyzer on new Salsa: master...davidbarsky:salsa:chayim-david-lukas/salsa-perf-improvements

Notably, these are without Chayim's enum changes being used in rust-analyzer, which I'll port over now.

[davidbarsky]

@davidbarsky As an alternative I have my branch (unpublished), that shrinks SyncState from 16 to 2 bytes, without hurting parallelism. I don't remember how much it saves compared to this branch, but I can check.

Feel free to check, but putting up the branch (even in a draft state!) would be great.

[ChayimFriedman2]

So I checked and on my machine, Lukas's changes save 255mb and my changes save 133mb. I'm working on finalizing and publishing my branch.

[davidbarsky]

For posterity/context for tomorrow's meeting, I believe Chayim's changes are on this branch: master...ChayimFriedman2:salsa:sync-v2.

[Veykril]

So, it would be nice for rust-analyzer to have this due to the memory decrease, I am not sure about the concurrency impact this has though. I would expect it to marginally be worse in that there is a coarser grained lock involved (within the ingredient), but that should only be an issue when there is high contention for the same ingredient so I believe this ought to be fine. (also adjusted the PR description)

[Veykril]

I'll draft this for now, I think we can consider this once r-a is properly moved over and more parallelized to see if this has an effect or not

[davidbarsky]

Reporting what Lukas said on Zulip:

The different is effectively:

Before:
Mutex per Memo guarding the vec of syncs per memo ingredient index

After:
Mutex per function ingredient guarding the hashmap of syncs per salsa struct ID

Notably the latter can ignore the memo ingredient index as it would be always the same due to the sync table being different per ingredient