Closed Bug 1373798 Opened 7 years ago Closed 7 years ago

stylo: Consider moving HTML dir attribute state into event state flags

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(6 files)

Performance testcase that should just share everything, so works revalidation hard
448.02 KB, text/html
Details
Bug 1373798 part 1. Stop calling SetHasDirAuto/ClearHasDirAuto in input element code.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1373798 part 2. Introduce event state flags that track the state of an element's "dir" attribute.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1373798 part 3. Rewrite our existing checks for the state of the "dir" attr on top of the new event state flags.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1373798 part 4. Add pseudo-classes for matching on the "dir" attribute states.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1373798 part 5. Use the new "dir" attribute pseudoclasses in html.css.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
We have rules in html.css that look like this:

[dir] { unicode-bidi: isolate; }
[dir="rtl"] { direction: rtl; }
[dir="ltr"] { direction: ltr; }
bdi:dir(ltr), [dir="auto"]:dir(ltr) { direction: ltr; }
bdi:dir(rtl), [dir="auto"]:dir(rtl) { direction: rtl; }

Those five selectors involving [dir] end up in revalidation selectors, and are a noticeable fraction of revalidation time in stylo, because we have to revalidate them for every single element.

Right now we have various node state flags that encode some of this dir attribute state.  Specifically, per bug 1331322 comment 10, we have:

    // Set if node has a dir attribute with a valid value (ltr, rtl, or auto)
    NodeHasValidDirAttribute,
    // Set if node has a dir attribute with a fixed value (ltr or rtl, NOT auto)
    NodeHasFixedDir,
    // Set if the node has dir=auto.
    NodeHasDirAuto,

We were considering consolidating those a bit (three bits to represent four states: "no dir, not bdi", "no dir, bdi", "dir=auto", "dir=rtl/ltr" is a bit silly; see bug 1338723).  But what we could do instead is move some of this state to event state flags and introduce new pseudo-classes that match on it to replace the selectors above.  Specifically, we would have 4 event state flags:

  HAS_DIR_ATTR
  DIR_ATTR_RTL
  DIR_ATTR_LTR
  DIR_ATTR_LIKE_AUTO

with this last set exactly when NodeHasDirAuto is set right now: on explicit dir="auto" or on bdi with no dir attr.  Then NodeHasValidDirAttribute can be recovered as "one of those last three is set" and NodeHasFixedDir can be recovered as "DIR_ATTR_RTL or DIR_ATTR_LTR is set".  And we can replace all the selectors above with pseudo-class selectors that map to those bits.

I did check what the spec says, and it does NOT have an equivalent to our "[dir] { unicode-bidi: isolate; }" rule.  Instead it has:

  [dir=ltr i], [dir=rtl i], [dir=auto i] {
    unicode-bidi: isolate; 
  }

so we could also update to that and get rid of the HAS_DIR_ATTR thing entirely.  I'm not sure about compat impact there, so filed https://github.com/whatwg/html/issues/2769 for the moment.
Blocks: 1338723
Blocks: 1373362
Load the testcase, then run this bookmarklet:

javascript:var%20e%20=%20document.documentElement;%20var%20s%20=%20e.style;%20s.display%20=%20"none";%20e.offsetWidth;%20var%20d%20=%20new%20Date;%20s.display%20=%20"";%20window.getComputedStyle(e,"").color;%20var%20d2%20=%20new%20Date;%20alert(d2%20-%20d);

and measure how much time is spent under StyleDocument.
Profile of the comment 1 testcase without these changes shows 20.2ms spent in matching revalidation selectors (out of 42.3ms under Servo_TraverseSubtree): https://perf-html.io/public/4029659c4fdd4d9778fc96f0e89e3868de1d6b94/calltree/?search=Servo_TraverseSubtree&thread=3

Profile of the comment 1 testcase with these changes shows 12.9ms spent in matching revalidation selectors (out of 33.4ms under Servo_TraverseSubtree, so the rest of the traversal is about the same): https://perf-html.io/public/d12e33103752cf30e0f956be2a0fe59daae75040/calltree/?search=Servo_TraverseSubtree&thread=4

So we're saving, on my hardware, about 8ms per 32000, elements, or about 1ms per 4000 elements.  So it's probably good for 1-5ms on your typical web page...
> Then NodeHasValidDirAttribute can be recovered as "one of those last three is set"

Not true, as the comments in the patch point out: the last flag is set for both <bdi dir="auto"> and <bdi dir="something-invalid"> but we want to set NodeHasValidDirAttribute differently for those two cases.
Comment on attachment 8878856 [details]
Bug 1373798 part 4.  Add pseudo-classes for matching on the "dir" attribute states.

https://reviewboard.mozilla.org/r/150116/#review154784

::: servo/components/style/gecko/generated/structs_debug.rs:16173
(Diff revision 1)
> -        NodeHasDirAutoSet = 21,
> -        NodeHasTextNodeDirectionalityMap = 22,
> -        NodeHasDirAuto = 23,
> -        NodeAncestorHasDirAuto = 24,
> -        ElementIsInStyleScope = 25,
> -        ElementIsScopedStyleRoot = 26,
> +        NodeHasTextNodeDirectionalityMap = 21,
> +        NodeAncestorHasDirAuto = 22,
> +        ElementIsInStyleScope = 23,
> +        ElementIsScopedStyleRoot = 24,
> +        NodeHandlingClick = 25,
> +        NodeHasRelevantHoverRules = 26,

This reminds me we should probably get rid of this flag, or finding a sane way to set it on stylo if we consider it's needed.
Attachment #8878856 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8878857 [details]
Bug 1373798 part 5.  Use the new "dir" attribute pseudoclasses in html.css.

https://reviewboard.mozilla.org/r/150118/#review154786

r=me
Attachment #8878857 - Flags: review?(emilio+bugs) → review+
> This reminds me we should probably get rid of this flag or finding a sane
> way to set it on stylo if we consider it's needed.

I filed bug 1374135 on this.
Comment on attachment 8878853 [details]
Bug 1373798 part 1.  Stop calling SetHasDirAuto/ClearHasDirAuto in input element  code.

https://reviewboard.mozilla.org/r/150110/#review155118
Attachment #8878853 - Flags: review?(michael) → review+
Comment on attachment 8878854 [details]
Bug 1373798 part 2.  Introduce event state flags that track the state of an element's "dir" attribute.

https://reviewboard.mozilla.org/r/150112/#review155120

::: dom/html/nsGenericHTMLElement.cpp:719
(Diff revision 1)
>            ClearHasDirAuto();
>            SetHasFixedDir();

The duplication here is a tad ugly, but it goes away in the next patch so I'm OK with it :).
Attachment #8878854 - Flags: review?(michael) → review+
Comment on attachment 8878855 [details]
Bug 1373798 part 3.  Rewrite our existing checks for the state of the "dir" attr on top of the new event state flags.

https://reviewboard.mozilla.org/r/150114/#review155122
Attachment #8878855 - Flags: review?(michael) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c8752c4b6fb
part 1.  Stop calling SetHasDirAuto/ClearHasDirAuto in input element  code.  r=mystor
https://hg.mozilla.org/integration/autoland/rev/a5dd7744170e
part 2.  Introduce event state flags that track the state of an element's "dir" attribute. r=mystor
https://hg.mozilla.org/integration/autoland/rev/dc19b4db7e51
part 3.  Rewrite our existing checks for the state of the "dir" attr on top of the new event state flags.  r=mystor
https://hg.mozilla.org/integration/autoland/rev/0970ac62b245
part 4.  Add pseudo-classes for matching on the "dir" attribute states.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/ef2e6aa3ae88
part 5.  Use the new "dir" attribute pseudoclasses in html.css.  r=emilio
Backed these out for browser_parseable_css.js failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108340131&repo=autoland


https://hg.mozilla.org/integration/autoland/rev/098e319682c4476127b7a9811ddd8fa66ed3d19f

Revert of the servo PR is working its way through the system.
Flags: needinfo?(bzbarsky)
Argh.  Gotta love broken tests.  :(

People should have just backed out the last changeset from autoland, for what it's worth, instead of reverting the servo PR...
I guess I'll just drop the plaintext.css optimization attempt for now.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a5065532b75
part 1.  Stop calling SetHasDirAuto/ClearHasDirAuto in input element  code.  r=mystor
https://hg.mozilla.org/integration/autoland/rev/c9e57de5c07f
part 2.  Introduce event state flags that track the state of an element's "dir" attribute. r=mystor
https://hg.mozilla.org/integration/autoland/rev/3947ddaad2c3
part 3.  Rewrite our existing checks for the state of the "dir" attr on top of the new event state flags.  r=mystor
https://hg.mozilla.org/integration/autoland/rev/b55bd07ee6b8
part 4.  Add pseudo-classes for matching on the "dir" attribute states.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/594416fea6e2
part 5.  Use the new "dir" attribute pseudoclasses in html.css.  r=emilio
> People should have just backed out the last changeset from autoland, for what it's worth

And yes, I do realize it may be hard to tell that from the changesets....
Flags: needinfo?(bzbarsky)
Assignee: nobody → bzbarsky
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: