Skip to content

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
@netlify
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

@codspeed-hq
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
Member

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
@github-actions github-actions bot mentioned this pull request Jun 3, 2025
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