Closed Bug 1455032 Opened 6 years ago Closed 6 years ago

Pack the shadow cascade order in ApplicableDeclarationBlock.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1454162 grew it by a word, but this number is unlikely to be more than 1, actually.
Comment on attachment 8968994 [details]
Bug 1455032: Pack the shadow cascade order in ApplicableDeclarationBlock.

So, shrinking from 24 to 20 bytes certainly gets closer to something that might be hit on realistic webpages. Blink is still at 18 of course, see [1].

Then it's also not clear to me how we feel about the shadow limit, which may need to be specced.

The size of ApplicableDeclarationBlock is important, and I do think we probably don't want to sacrifice it here. But we may want to be sure the appropriate spec issues are raised. Bumping this review to Cam.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=732502
Attachment #8968994 - Flags: review?(bobbyholley) → review?(cam)
See also bug 1371949 for some context around the current setup.
If we do want to stick with 24 bits for source order, and I guess we should try to avoid reducing it if we can, then there are free bits around for us to use.  There are 2 at the top of the specificity u32 (since we only use 10 + 10 + 10 of them), and there are 4 at the top of the CascadeLevel byte, since we've still got fewer than 16 levels.

Bobby, how important is it that level() is a just single byte load?  If it would be fine to AND off some bits after that load, then I would suggest packing the 4 bits of shadow limit into the top of the CascadeLevel byte.

I guess I'm not super worried about having a small-ish shadow limit, especially given this is not just "things will break if you have shadow hosts of this depth" but "things will break if you take an element and slot it down through this number of shadow hosts and you have matching ::slotted rules in multiple shadow depths beyond that limit".  But maybe we should grab those extra two bits in the specificity byte to bump it up to 6 bits of shadow limit.

As for whether this should be specified -- there are other things in CSS specs where impementation defined limits apply, such as number of bits to use in the specificity components.  We could file an issue to explicitly allow it here too, but I'm not sure it's worth it.


(We could pack ApplicableDeclaration block down to 16 bytes, really, if we were happy to manage the StyleSource enum ourselves and store its tagging bit, to distinguish whether it's a Arc<Locked<StyleRule>> or Arc<Locked<PropertyDeclarationBlock>>, elsewhere.)
Flags: needinfo?(bobbyholley)
(In reply to Cameron McCormack (:heycam) from comment #4)
> Bobby, how important is it that level() is a just single byte load?  If it
> would be fine to AND off some bits after that load, then I would suggest
> packing the 4 bits of shadow limit into the top of the CascadeLevel byte.

Adding a mask operation after the load should be fine. In general, this stuff is all gated on memory bandwidth, and an extra ALU operation here and there is rarely measurable even on the hottest paths, assuming it doesn't create any hard-to-predict branch behavior.

> (We could pack ApplicableDeclaration block down to 16 bytes, really, if we
> were happy to manage the StyleSource enum ourselves and store its tagging
> bit, to distinguish whether it's a Arc<Locked<StyleRule>> or
> Arc<Locked<PropertyDeclarationBlock>>, elsewhere.)

Hey, that's a nice idea. If I find an extra minute I'll write up some generic tagged-pointer stuff in servo_arc and use it to shrink StyleSource.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #5)
> Hey, that's a nice idea. If I find an extra minute I'll write up some
> generic tagged-pointer stuff in servo_arc and use it to shrink StyleSource.

Started this in bug 1455784.
Comment on attachment 8968994 [details]
Bug 1455032: Pack the shadow cascade order in ApplicableDeclarationBlock.

https://reviewboard.mozilla.org/r/237694/#review244490

Let's use more of the available bits per comment 4.
Attachment #8968994 - Flags: review?(cam) → review-
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Comment on attachment 8968994 [details]
Bug 1455032: Pack the shadow cascade order in ApplicableDeclarationBlock.

https://reviewboard.mozilla.org/r/237694/#review244856

Looks good, thanks!

::: servo/components/style/applicable_declarations.rs:27
(Diff revision 2)
> -/// Note that the value of 24 is also hard-coded into the level() accessor,
> +/// Note that the value of 24 is hard-coded into the level() accessor,
>  /// which does a byte-aligned load of the 4th byte. If you change this value
>  /// you'll need to change that as well.

We can drop this comment now that we no longer do the single byte load.

::: servo/components/style/applicable_declarations.rs:52
(Diff revision 2)
> +const CASCADE_LEVEL_SHIFT: usize = SOURCE_ORDER_BITS + SHADOW_CASCADE_ORDER_BITS;
> +const CASCADE_LEVEL_BITS: usize = 4;
> +const CASCADE_LEVEL_MAX: u8 = (1 << CASCADE_LEVEL_BITS) - 1;
> +const CASCADE_LEVEL_MASK: u32 = (CASCADE_LEVEL_MAX as u32) << CASCADE_LEVEL_SHIFT;
>  
>  /// Stores the source order of a block and the cascade level it belongs to.

Mention the shadow cascade order too.

::: servo/components/style/applicable_declarations.rs:67
(Diff revision 2)
> +        debug_assert!(
> +            cascade_level as u8 <= CASCADE_LEVEL_MAX,
> +            "Gotta find more bits!"
> +        );
>          let mut bits = ::std::cmp::min(source_order, SOURCE_ORDER_MAX);
> -        bits |= (cascade_level as u8 as u32) << SOURCE_ORDER_BITS;
> +        bits |= ((shadow_cascade_order & SHADOW_CASCADE_ORDER_MAX) as u32) << SHADOW_CASCADE_ORDER_SHIFT;

Is it worth doing a min() here too, to get slightly more understandable results in case of out of range shadow_cascade_order values?

::: servo/components/style/applicable_declarations.rs:73
(Diff revision 2)
> -        SourceOrderAndCascadeLevel(bits)
> +        bits |= (cascade_level as u8 as u32) << CASCADE_LEVEL_SHIFT;
> +        ApplicableDeclarationBits(bits)
>      }
>  
> -    fn order(&self) -> u32 {
> +    fn source_order(&self) -> u32 {
>          self.0 & SOURCE_ORDER_MASK

Maybe do (... >> SOURCE_ORDER_SHIFT)?
Attachment #8968994 - Flags: review?(cam) → review+
Comment on attachment 8968994 [details]
Bug 1455032: Pack the shadow cascade order in ApplicableDeclarationBlock.

https://reviewboard.mozilla.org/r/237694/#review244856

> Is it worth doing a min() here too, to get slightly more understandable results in case of out of range shadow_cascade_order values?

We get that behavior already, given it can only increase, and the only usage of it only does something if the new value is strictly greater than the previous.

> Maybe do (... >> SOURCE_ORDER_SHIFT)?

Good call :)
Comment on attachment 8968994 [details]
Bug 1455032: Pack the shadow cascade order in ApplicableDeclarationBlock.

https://reviewboard.mozilla.org/r/237694/#review244856

> We get that behavior already, given it can only increase, and the only usage of it only does something if the new value is strictly greater than the previous.

Clarifying.. if it wraps around, it'll only get treated as the max level so far.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6538479958ef
Pack the shadow cascade order in ApplicableDeclarationBlock. r=heycam
https://hg.mozilla.org/mozilla-central/rev/6538479958ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.