Lazily allocate extra memo state #888
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
ibraheemdev
force-pushed
the
ibraheem/lazy-memos
branch
5 times, most recently
from
24d561e to
11fd385
Compare
CodSpeed Performance ReportMerging #888 will not alter performanceComparing Summary
|
|
I'm tempted to also throw in |
There was a problem hiding this comment.
This is great. I wonder if it would simplify the code if QueryRevisionsExtra would abstract the Option<Box<Data>> internally. Curious to hear your perspective.
I think we could also move AccumulatedValues. Let me know if you want to do this in the same or a separate PR
src/active_query.rs
Outdated
| let extra = QueryRevisionsExtra { | ||
| tracked_struct_ids: mem::take(tracked_struct_ids), | ||
| cycle_heads: mem::take(cycle_heads), | ||
| }; |
There was a problem hiding this comment.
Nit: I think it would be nice if QueryRevisionsExtra could abstract the fact that it is an Option<Box<Data>> internally to keep the code here simpler (QueryRevisions::new(final, tracked_struct_ids, cycle_heads))
src/function.rs
Outdated
| @@ -191,7 +191,10 @@ where | |||
| mut memo: memo::Memo<C::Output<'db>>, | |||
| memo_ingredient_index: MemoIngredientIndex, | |||
| ) -> &'db memo::Memo<C::Output<'db>> { | |||
| memo.revisions.tracked_struct_ids.shrink_to_fit(); | |||
| if let Some(extra) = &mut memo.revisions.extra { | |||
There was a problem hiding this comment.
Same here. I think I'd prefer if the call is memo.revisions.extra.shrink_to_fit
src/zalsa_local.rs
Outdated
| @@ -328,6 +328,30 @@ pub(crate) struct QueryRevisions { | |||
| /// How was this query computed? | |||
| pub(crate) origin: QueryOrigin, | |||
|
|
|||
| pub(super) accumulated: Option<Box<AccumulatedMap>>, | |||
There was a problem hiding this comment.
We could probably move AccumulatedMap too
I assume you didn't move accumualted_inputs because it fits into what otherwise would be padding (similar to verified_final)
There was a problem hiding this comment.
I'd say lets move accumulated map in there as well, given we already have it lazily allocated (and neither you folks or rust-analyzer seem to use it much at the moment). In fact I still don't see a future where r-a wants to use it tbh 😅
Though I guess moving that in there as well makes using any of the other features (cycles, tracked structs) more expensive again.
ibraheemdev
force-pushed
the
ibraheem/lazy-memos
branch
3 times, most recently
from
af9f582 to
4fb2520
Compare
ibraheemdev
force-pushed
the
ibraheem/lazy-memos
branch
from
4fb2520 to
637fb28
Compare
Similar to #768, except also lazily allocating the
CycleHeads. I did some profiling in ty, and on a cold run ~1% of memos have non-emptyCycleHeadsand ~0.5% create tracked structs, out of >2M memos on large projects. Lazily allocating saves a meaningful amount of memory for us (4 words on memos without extras) and doesn't affect performance for ty (if anything it seems to improve performance very slightly, but that may just be noise).