-
Notifications
You must be signed in to change notification settings - Fork 175
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
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #816 will not alter performanceComparing Summary
|
327f336
to
1ac896e
Compare
src/function/execute.rs
Outdated
@@ -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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
1ac896e
to
b5e194d
Compare
Notable we do not do this for cycles, given there the memo is likely temporary anyways