perf/fix: retain mut algorithm - #519
Conversation
* fixes panic behavior to rust standard library * improves performance of retain algorithm by reducing memory copies
|
yeah improvements are crazy |
| original_len | ||
| }; | ||
| // SAFETY: previous `read` is always less than original_len. | ||
| unsafe { drop_in_place(&mut *g.v.as_mut_ptr().add(read)) }; |
There was a problem hiding this comment.
why not use the method instead here??
unsafe { g.v.as_mut_ptr().add(read).drop_in_place() }codegen is probably the same but it avoids dereferencing and lets the machinery do whatever it needs to do
There was a problem hiding this comment.
or does codegen see something because it sees it's not a null pointer??
it shouldn't
There was a problem hiding this comment.
or why not even
unsafe { drop_in_place(g.v.as_mut_ptr().add(read)) };
|
the only downside is that the algorithm is significantly more complex but it is worthwhile I'll re-review this tomorrow, these hours are not a good time to be reasoning about unsafe rust, but it looks good |
|
@TDecking what are your thoughts on this |
TDecking
left a comment
TDecking
left a comment
There was a problem hiding this comment.
This PR is a port of a std::Vec::retain_mut change done several month earlier. It should be fine to include.
| // SAFETY: `read` is always less than original_len. | ||
| let ptr = g.v.as_mut_ptr(); | ||
| let cur = unsafe { &mut *ptr.add(g.read) }; |
There was a problem hiding this comment.
This change from the main Rust implementation separated the safety comment from the unsafe block. Please fix it.
| // This drop guard will be invoked when predicate or `drop` of element | ||
| // panicked. It shifts unchecked elements to cover holes and | ||
| // `set_len` to the correct length. In cases when predicate and | ||
| // `drop` never panick, it will be optimized out. |
There was a problem hiding this comment.
| // `drop` never panick, it will be optimized out. | |
| // `drop` never panics, it will be optimized out. |
It also inside the main Rust repository.
3 participants
Closes #444
benchmark results shows about 20-30% performance improvement in cases that retain actually happens: