Skip to content

refactor: Shrink QueryRevisions by 3 usize by boxing IdentityMap #768

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

refactor: Shrink QueryRevisions by 3 usize by boxing IdentityMap #768

wants to merge 1 commit into from

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Mar 20, 2025

Not all queries actually create tracked structs and so this may be empty in those cases

@netlify Netlify
Copy link

netlify bot commented Mar 20, 2025 •
edited
Loading

✅ Deploy Preview for salsa-rs canceled.

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

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Mar 20, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #768 will not alter performance

Comparing Veykril:veykril/push-zuotrtpokwsz (3c13bf0) with master (20a347e)

Summary

✅ 12 untouched benchmarks

davidbarsky
davidbarsky approved these changes Mar 21, 2025
View reviewed changes
Copy link
Contributor

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

lgtm, modulo doc comment nit

@Veykril Veykril enabled auto-merge March 21, 2025 18:28
@Veykril Veykril force-pushed the veykril/push-zuotrtpokwsz branch 2 times, most recently from 9bf0dfb to 20f8e85 Compare March 22, 2025 09:02
@MichaReiser
Copy link
Contributor

MichaReiser commented Mar 22, 2025 •
edited
Loading

Do you see any improvement of peak memory consumption in r-a with this change? I'm just curious on how we make the decisions on perf vs memory consumption (both are very important to us and trading perf for memory consumption can certainly be a worth trade-off, it's just that I don't see the memory impact from our CI pipeline)

@Veykril
Copy link
Member Author

Veykril commented Mar 22, 2025 •
edited
Loading

I have not yet, I agree for this one specifically we should as it has performance implications. We'll need to move r-a to the fixpoint changes, then we can test this PR out on our stats afterwards. (though no the regression reported here is not from this PR specifically, that bench regressed on a bunch of PRs randomly)

@Veykril
refactor: Shrink QueryRevisions by 3 usize by boxing IdentityMap
3c13bf0 
Not all queries actually create tracked structs and so this may be empty in those cases
@Veykril Veykril force-pushed the veykril/push-zuotrtpokwsz branch from 20f8e85 to 3c13bf0 Compare March 26, 2025 06:29
@Veykril Veykril added this pull request to the merge queue Mar 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2025
@Veykril
Copy link
Member Author

Veykril commented Mar 26, 2025 •
edited
Loading

image
I am not sure what happened here, I am fairly certain I did not press the button there 🤨

Edit: Ah this was still queued up from a couple days ago

@Veykril Veykril marked this pull request as draft March 26, 2025 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbarsky davidbarsky davidbarsky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Veykril @MichaReiser @davidbarsky