Pack QueryOrigin memory layout
#885
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
ibraheemdev
force-pushed
the
ibraheem/packed-query-origins
branch
from
275b8ed to
8de13f5
Compare
CodSpeed Performance ReportMerging #885 will improve performances by 11.21%Comparing Summary
Benchmarks breakdown
|
| let memo_ingredient_index = self.memo_ingredient_index(zalsa, key); | ||
| self.get_memo_from_table_for(zalsa, key, memo_ingredient_index) | ||
| .map(|m| m.revisions.origin.clone()) |
There was a problem hiding this comment.
I believe we were cloning here unnecessarily. Removing this is likely the reason for the accumulator benchmark improvement.
ibraheemdev
force-pushed
the
ibraheem/packed-query-origins
branch
from
8de13f5 to
cc6e38e
Compare
| /// SAFETY: Same as above. | ||
| unsafe impl Sync for QueryOriginData {} | ||
|
|
||
| impl QueryOrigin { |
There was a problem hiding this comment.
now, I understand this might be a bit much to ask, but how would you feel about pulling in arbtest (it's my current favorite property testing library...) and generating sequences of method calls in this style? I know the unsafe code is probably fine, but it'd be nice to cut down the amount of "eh, i guess it's fine" unsafe code.
There was a problem hiding this comment.
I'm not really sure how useful property testing here would be compared to unit tests, but I'm also pretty happy with our current Miri coverage so I don't know if it's worth investing time in. I'm happy to review a PR if you want to add something though :)
Co-authored-by: David Barsky <[email protected]>
ibraheemdev
force-pushed
the
ibraheem/packed-query-origins
branch
from
8a0f4b8 to
0f178b7
Compare
Packs
QueryOrigininto a(u64, u32, u8)union layout to take advantage of the padding bytes inQueryRevisions. This cuts down the size of memos by two words.