Change swap to memcopy in Column::initialize_from_unchecked#24312
Change swap to memcopy in Column::initialize_from_unchecked#24312SpecificProtagonist wants to merge 5 commits into
Conversation
ae474b2 to
ce3bb35
Compare
ce3bb35 to
f2b7ea2
Compare
amtep
left a comment
There was a problem hiding this comment.
I don't feel qualified to approve unsafe code but I reviewed it anyway
chescock
left a comment
There was a problem hiding this comment.
This looks correct to me! We should benchmark before merging, but I would expect it to help.
| &mut self, | ||
| other: &mut Column, | ||
| other_last_element_index: usize, | ||
| src: &mut Column, |
There was a problem hiding this comment.
I like that this makes the name more consistent with src_row!
Huh, now it's more obvious that the columns were dst, src, while the rows were src, dst. We definitely shouldn't change that in this PR, but I wonder whether it would be more clear to eventually move these to methods on src so that the order is consistent.
|
I'm having trouble getting consistent results from the add_remove benchmarks. For example, this is a result of this PR vs main: ResultsBut then re-running it with the same saved baseline showed add_remove_very_big at a 30% improvement instead. It's probably heavily influenced by where in memory the allocations end up at. |
Objective
When moving a component between tables, we move it to the end of the table before moving it to the new table; we can just move them directly to the new table.
Solution
I don't expect noticeable gains, but it doesn't make the code more complex. I'm not an expert in reading assembly and haven't benched this yet, but
Column::initialize_from_uncheckednow is half as long and callsmemcpyinstead ofmemmove.