1370711 - stylo: Force the NonZero optimization for servo_arc and StrongRuleNode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Bobby Holley (:bholley)]

Fixing this brings the size of ElementData from 88 bytes to 56 bytes. Credit to nox for prodding me to do this.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add some size tests for ElementData and what's inside. v1

MozReview-Commit-ID: Aj0rryEnJpl

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make a bunch of types 'static. v1

The reasoning for this is explained in a comment in the next patch.

MozReview-Commit-ID: FQgDY77mg3B

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Introduce NonZeroPtrMut and use it in servo_arc. v1

MozReview-Commit-ID: AAmeyjfyXeU

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Use NonZeroPtrMut for {Strong,Weak}RuleNode. v1

MozReview-Commit-ID: Da4Ds7SIcCD

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875046 [details] [diff] [review]
Part 3 - Introduce NonZeroPtrMut and use it in servo_arc. v1

Review of attachment 8875046 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/servo_arc/lib.rs
@@ +79,5 @@
> +/// be thin or fat (which depends on whether or not T is sized). Given
> +/// that this is all a temporary hack, this restriction is fine for now.
> +///
> +/// [1] https://github.com/rust-lang/rust/issues/27730
> +pub struct NonZeroPtrMut<T: ?Sized + 'static>(&'static mut T);

The `mut` shouldn't be necessary. If this is an Arc it can't be mutated anyway, except for the internal atomic, which rust's aliasing analysis understands as always-mutable.

@@ +97,5 @@
> +        NonZeroPtrMut::new(self.ptr())
> +    }
> +}
> +
> +impl<T: ?Sized + 'static> ::std::fmt::Debug for NonZeroPtrMut<T> {

This should be an impl of http://doc.servo.org/core/fmt/trait.Pointer.html (`{:p}`), and the Debug impl should pass-through or something. Probably.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #5)
> Comment on attachment 8875046 [details] [diff] [review]
> Part 3 - Introduce NonZeroPtrMut and use it in servo_arc. v1
> 
> Review of attachment 8875046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/servo_arc/lib.rs
> @@ +79,5 @@
> > +/// be thin or fat (which depends on whether or not T is sized). Given
> > +/// that this is all a temporary hack, this restriction is fine for now.
> > +///
> > +/// [1] https://github.com/rust-lang/rust/issues/27730
> > +pub struct NonZeroPtrMut<T: ?Sized + 'static>(&'static mut T);
> 
> The `mut` shouldn't be necessary. If this is an Arc it can't be mutated
> anyway, except for the internal atomic

Well, modulo make_mut, and types with internal mutability like AtomicRefCell.

>, which rust's aliasing analysis
> understands as always-mutable.

Yeah, but the pointers were *mut before. I'd rather just match the old code as closely as possible, since I don't think it hurts anything.

> 
> @@ +97,5 @@
> > +        NonZeroPtrMut::new(self.ptr())
> > +    }
> > +}
> > +
> > +impl<T: ?Sized + 'static> ::std::fmt::Debug for NonZeroPtrMut<T> {
> 
> This should be an impl of http://doc.servo.org/core/fmt/trait.Pointer.html
> (`{:p}`), and the Debug impl should pass-through or something. Probably.

Ok.

Comment 7

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/17197

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks like this has been merged:

https://hg.mozilla.org/mozilla-central/rev/ced7540c6f6f6cb857df6b3770121f6fceac726c