Skip to content

perf: Reduce memory usage by deduplicating type information #803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 22, 2025

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Apr 20, 2025

No description provided.

ChayimFriedman2 and others added 3 commits April 17, 2025 18:48
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.
Copy link

netlify bot commented Apr 20, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit ea12b6e
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/6805248a0f543f000863b1af

Copy link

codspeed-hq bot commented Apr 20, 2025

CodSpeed Performance Report

Merging #803 will degrade performances by 6.69%

Comparing Veykril:veykril/push-ukowpnqompwy (ea12b6e) with master (ab7ecb4)

Summary

❌ 1 (👁 1) regressions
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 amortized[SupertypeInput] 4 µs 4.2 µs -6.69%

@Veykril Veykril force-pushed the veykril/push-ukowpnqompwy branch from 0fdb778 to 13a448e Compare April 20, 2025 16:10
@Veykril
Copy link
Member Author

Veykril commented Apr 20, 2025

Perf comparison to #649, wanna check how things look without the const type id stuff
Looks like by not using the const type id stuff (and inlining one specific function) perf is in the same ballpark as #649

@Veykril Veykril force-pushed the veykril/push-ukowpnqompwy branch from 13a448e to ea12b6e Compare April 20, 2025 16:44
@ChayimFriedman2
Copy link
Contributor

Given that this has the same performance and less unsafe code, let's close #649 and merge this.

@Veykril
Copy link
Member Author

Veykril commented Apr 20, 2025

I can push my branch to yours if you want to retain the PR authorship

@ChayimFriedman2
Copy link
Contributor

I can push my branch to yours if you want to retain the PR authorship

I don't care.

@Veykril Veykril marked this pull request as ready for review April 21, 2025 04:23
@Veykril Veykril changed the title Reduce memory usage by deduplicating type information (copy) Reduce memory usage by deduplicating type information Apr 21, 2025
@Veykril
Copy link
Member Author

Veykril commented Apr 22, 2025

I'll go ahead and merge this given most of the work was by Chayim which I have reviewed. Perf is within noise and the memory savings are real. We'd like to upgrade our salsa today where having this in it would be pretty nice.

@Veykril Veykril added this pull request to the merge queue Apr 22, 2025
@Veykril Veykril changed the title Reduce memory usage by deduplicating type information perf: Reduce memory usage by deduplicating type information Apr 22, 2025
Merged via the queue into salsa-rs:master with commit f981e7d Apr 22, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-ukowpnqompwy branch April 22, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants