Skip to content

shrink_to_fit IdentityMap before storing it #816

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 1 commit into from
May 26, 2025

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Apr 24, 2025

Notable we do not do this for cycles, given there the memo is likely temporary anyways

Copy link

netlify bot commented Apr 24, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit b5e194d
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/683464307fe53d0008d6dfa9

Copy link

codspeed-hq bot commented Apr 24, 2025

CodSpeed Performance Report

Merging #816 will not alter performance

Comparing Veykril:veykril/push-owxlvpvmzzmn (b5e194d) with master (2a54667)

Summary

✅ 12 untouched benchmarks

@Veykril Veykril marked this pull request as ready for review May 26, 2025 11:05
@Veykril Veykril force-pushed the veykril/push-owxlvpvmzzmn branch from 327f336 to 1ac896e Compare May 26, 2025 11:05
@@ -105,6 +105,7 @@ where
// outputs and update the tracked struct IDs for seeding the next revision.
self.diff_outputs(zalsa, database_key_index, old_memo, &mut revisions);
}
revisions.tracked_struct_ids.shrink_to_fit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this call to Memo::new? It would then also apply for memos participating in cycles

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure if we want to. Since some of the cycle memos are only temporary, and so doing that isn't necessarily useful (it may incur extra unnecessary re-allocations)

Copy link
Contributor

Choose a reason for hiding this comment

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

This only applies to cycle heads (not queries participating in a cycle) and every cycle is very expensive anyway.

@MichaReiser MichaReiser added the refactoring Code works but is messy label May 26, 2025
@Veykril Veykril force-pushed the veykril/push-owxlvpvmzzmn branch from 1ac896e to b5e194d Compare May 26, 2025 12:53
@Veykril Veykril enabled auto-merge May 26, 2025 12:53
@Veykril Veykril added this pull request to the merge queue May 26, 2025
Merged via the queue into salsa-rs:master with commit b1b8b43 May 26, 2025
12 checks passed
@Veykril Veykril deleted the veykril/push-owxlvpvmzzmn branch May 26, 2025 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code works but is messy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants