Skip to content

Store tracked struct ids as ThinVec on Revisions #892

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 7 commits into from
Jun 3, 2025

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 31, 2025

We don't need a map. All we use it for is to seed new ids. This helps to reduce the QueryRevisionsExtra size (which will make up for the regression that I introduce in #882)

@MichaReiser MichaReiser marked this pull request as draft May 31, 2025 14:18
Copy link

netlify bot commented May 31, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit a6231f3
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/683d96378d48450008a91360

Copy link

codspeed-hq bot commented May 31, 2025

CodSpeed Performance Report

Merging #892 will not alter performance

Comparing MichaReiser:revisions-identity-vec (a6231f3) with master (14318b7)

Summary

✅ 12 untouched benchmarks

@MichaReiser MichaReiser force-pushed the revisions-identity-vec branch from 513a567 to d6bd400 Compare May 31, 2025 14:21
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be a Box<[T]> even? I don't think we ever push into it?

@MichaReiser
Copy link
Contributor Author

MichaReiser commented May 31, 2025

Can't this be a Box<[T]> even? I don't think we ever push into it?

Is it possible to delete from a box slice? We use retain. We could convert to a Vec and then back to a Boxed slice but that could be heavy on allocations

https://github.com/MichaReiser/salsa/blob/80655d09e691fb76fc0173cceeb9a08ed278b306/src/function/diff_outputs.rs#L53-L59

@MichaReiser
Copy link
Contributor Author

I guess, we could use a ThinVec

@MichaReiser MichaReiser marked this pull request as ready for review May 31, 2025 14:44
@MichaReiser MichaReiser changed the title Store tracked struct ids as Vec on Revisions Store tracked struct ids as ThinVec on Revisions May 31, 2025
@MichaReiser MichaReiser force-pushed the revisions-identity-vec branch from 9def062 to ff3585b Compare June 1, 2025 09:00
@ibraheemdev
Copy link
Contributor

This looks good to me, I was trying to refactor the code to avoid the retain call and instead filter the tracked struct IDs when we convert the map to a list. That way we could use Box<[_]> instead, but I'm not sure if that's possible.

@MichaReiser
Copy link
Contributor Author

This looks good to me, I was trying to refactor the code to avoid the retain call and instead filter the tracked struct IDs when we convert the map to a list. That way we could use Box<[_]> instead, but I'm not sure if that's possible.

That would be nice! It might still be worth it from a perf perspective to avoid the linear traversal

@MichaReiser MichaReiser force-pushed the revisions-identity-vec branch from a555a0a to 9ae5e76 Compare June 2, 2025 12:16
@MichaReiser MichaReiser added this pull request to the merge queue Jun 3, 2025
Merged via the queue into salsa-rs:master with commit 79c2eac Jun 3, 2025
12 checks passed
@MichaReiser MichaReiser deleted the revisions-identity-vec branch June 3, 2025 08:19
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.

3 participants