Repository: servo/servo
File: components/script_bindings/root.rs (Rust)

Code before:

impl RootCollection {
    /// Create an empty collection of roots
    #[allow(clippy::new_without_default)]
    pub const fn new() -> RootCollection {
        RootCollection {
            roots: UnsafeCell::new(vec![]),
        }
    }

    /// Starts tracking a trace object.
    unsafe fn root(&self, object: *const dyn JSTraceable) {
        assert_in_script();
        unsafe { (*self.roots.get()).push(object) };
    }

    /// Stops tracking a trace object, asserting if it isn't found.
    unsafe fn unroot(&self, object: *const dyn JSTraceable) {
        assert_in_script();
        let roots = unsafe { &mut *self.roots.get() };
        match roots
            .iter()
            .rposition(|r| std::ptr::addr_eq(*r as *const (), object as *const ()))
        {
            Some(idx) => {
                unsafe {
                    let len = roots.len() - 1;
                    // Partial inlining of `Vec::swap_remove` to avoid having to read
                    // and return the value since we don't care about it.
                    if len != idx {
                        let base_ptr = roots.as_mut_ptr();
                        ptr::copy_nonoverlapping(base_ptr.add(len), base_ptr.add(idx), 1);
                    }
                    roots.set_len(len);
                }
            },
            None => panic!("Can't remove a root that was never rooted!"),
        }
    }
}

thread_local!(pub static STACK_ROOTS: RootCollection = const { RootCollection::new() });

/// SM Callback that traces the rooted reflectors
///
/// # Safety
/// tracer must point to a valid, non-null JS tracer object.
pub unsafe fn trace_roots(tracer: *mut JSTracer) {
    trace!("tracing stack roots");
    STACK_ROOTS.with(|collection| {
        let collection = unsafe { &*collection.roots.get() };

Review comment (suggestion):
Can you add a comment here, that "this is identical to standard `swap_remove` but doing less work for our case"?

I hope to merge this.

Code after:

impl RootCollection {
    /// Create an empty collection of roots
    #[allow(clippy::new_without_default)]
    pub const fn new() -> RootCollection {
        RootCollection {
            roots: UnsafeCell::new(vec![]),
        }
    }

    /// Starts tracking a trace object.
    unsafe fn root(&self, object: *const dyn JSTraceable) {
        assert_in_script();
        unsafe { (*self.roots.get()).push(object) };
    }

    /// Stops tracking a trace object, asserting if it isn't found.
    unsafe fn unroot(&self, object: *const dyn JSTraceable) {
        assert_in_script();
        let roots = unsafe { &mut *self.roots.get() };
        match roots
            .iter()
            .rposition(|r| std::ptr::addr_eq(*r as *const (), object as *const ()))
        {
            Some(idx) => {
                // Partial inlining of `Vec::swap_remove` to avoid having to read and return the value since
                // we don't care about it, doing less work in our case.
                // SAFETY: the copy source and destination are derived from valid positions in the vector.
                unsafe {
                    let len = roots.len() - 1;
                    if len != idx {
                        let base_ptr = roots.as_mut_ptr();
                        ptr::copy_nonoverlapping(base_ptr.add(len), base_ptr.add(idx), 1);
                    }
                    roots.set_len(len);
                }
            },
            None => panic!("Can't remove a root that was never rooted!"),
        }
    }
}

thread_local!(pub static STACK_ROOTS: RootCollection = const { RootCollection::new() });

/// SM Callback that traces the rooted reflectors
///
/// # Safety
/// tracer must point to a valid, non-null JS tracer object.
pub unsafe fn trace_roots(tracer: *mut JSTracer) {
    trace!("tracing stack roots");
    STACK_ROOTS.with(|collection| {