script: inline roots.swap_remove() #40777
Conversation
also worth replacing it with unsafe code? |
|
Thanks for your performance investigation work! Does this improve any benchmarks? I wouldn't be surprised if the compiler already performed this optimization. |
I spotted the |
| Some(idx) => { | ||
| roots.swap_remove(idx); | ||
| unsafe { | ||
| let len = roots.len() - 1; |
There was a problem hiding this comment.
why? If we are in this branch, we found a match so roots.len() >= 1 and then len >= 0 . Or am I missing something?
There was a problem hiding this comment.
You are right. I was checking this only with https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#2048 in mind without considering too much context.
| let base_ptr = roots.as_mut_ptr(); | ||
| ptr::copy_nonoverlapping(base_ptr.add(len), base_ptr.add(idx), 1); | ||
| } | ||
| roots.set_len(len); |
There was a problem hiding this comment.
We shouldn't set new length here. It should be part of above if block.
There was a problem hiding this comment.
I disagree: we always need to update the length, but we can skip the copy/move when the match it the last element.
| Some(idx) => { | ||
| roots.swap_remove(idx); | ||
| unsafe { | ||
| let len = roots.len() - 1; |
There was a problem hiding this comment.
You are right. I was checking this only with https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#2048 in mind without considering too much context.
jdm
left a comment
There was a problem hiding this comment.
I think there's an opportunity to move this code into a reusable function that can be called from some other code with similar usage patterns, but I'll file an issue about that.
This saves the work done by 'ptr::read()' that we don't use. Signed-off-by: webbeef <me@webbeef.org>
4742d67 to
b7120a8
Compare
This saves the work done by 'ptr::read()' that we don't use.
Testing: Green WPT run: https://github.com/webbeef/servo/actions/runs/19560614891