Closed Bug 1425964 Opened 3 years ago Closed 2 years ago

isDocumentRTL forces multiple restyles.

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

isDocumentRTL forces multiple restyles.
On this profile: https://perfht.ml/2BKo5Wo
We see isDocumentRTL called 8 times, thus, forcing 8 synchronous restyles.
A total of 290ms on 8322ms for the whole action = 3.4%.

DAMP reports no win when trying to cache isDocumentRTL in order to do only one sync restyle:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=a11f5bd42b466ce7f64a815f6e9cd0ddab92a575&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmo&framework=1&selectedTimeRange=172800

In theory, it should report a win on complicated.requestsFinished.
A 3% win should easily be reported as the current noise of this test is around 0.6%.
Severity: normal → enhancement
Priority: -- → P3
Maybe something worth to try: do everything in CSS with https://developer.mozilla.org/en-US/docs/Web/CSS/:dir. Maybe we can draw the canvas always in the same direction, but mirror it in CSS using that :dir pseudoclass.

Another possibility is to force the "dir" attribute on the document at startup (for sure we should be able to set it from Firefox ?) so that we can just get it.
Ok, I finally understood why DAMP was not reporting a win.
That's because the waterfall doesn't render while running DAMP test.
I'll open a bug to improve DAMP coverage.
Depends on: 1426206
profile with DAMP fix for waterfall *AND* new custom.netmonitor test:
  https://perfht.ml/2BLmJuA
  WaterfallBackground.draw takes 200ms during requestsFinished which takes 8813ms
  So we spend 2% of time within requestsFinished to execute "draw" function.

Same patch queue, but with the waterfall isRTL fix:
  https://perfht.ml/2AZPUXi
  Now, draw takes 120ms and requestsFinished takes 13514ms on this run (my computer may have been busier than the previous run).
  What matter is that "draw" now represents only 0.8% of requestsFinished test execution.

So, in theory, we should see a 2 - 0.8 = 1.2% improvement on DAMP's requestsFinished.
The noise of requestsFinished is around 1.2-1.4%, so we still don't see any significant win here:
  https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=cbb06e66013e5e1252c7ab84b9d1f7581c4a76c0&newProject=try&newRevision=3f88fa39d327e70f4a43f6b078125fe7b3ec6941&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Ok, I finally understood why DAMP was not reporting a win.
> That's because the waterfall doesn't render while running DAMP test.
> I'll open a bug to improve DAMP coverage.
Nice, just R+ed the patch. 

Honza
Actually I'm even wondering why we need getComputedStyle at all. Why can't we just get `document.dir` and check if it's "rtl"?
I downloaded Nightly in arabic to check: document.dir is "rtl" in the netmonitor frame, while it's "ltr" on an english Nightly. Then I believe we can use it directly.

Note: during the all hands, I've seen that our localization team could use pseudo locales, I asked them some guidance about it, as it would be a lot easier to use an RTL pseudo-locale than an arabic locale ;)
Assignee: nobody → poirot.alex
As we just discussed on slack with Julian, document.dir is a bit special.
It works in panel's documents thanks to bug 1305007, without this, document.dir would be empty.
It is automagically set by toolbox code on all panels except XUL ones, or HTML docs that have not set any "dir" attribute on their <html>. So it doesn't work for Performance, styleditor and storage.
It doesn't work for toolbox document either as it is still XUL and we can't set document.dir correctly for XUL docs.

Anyway, since we do our best to set document.dir correctly, I think it makes sense to use it everywhere we can to prevent costly reflows. The first patch only optimize toolbox code to do a reflow only once. I see a very small win via the profile (0.3ms down to 0.1ms) for setIframeDocumentDir. The second patch converts all getComputedStyle usages related to direction except for HTMLTooltip which may involve Markup view iframe document, which doesn't get its document.dir set correctly.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f87f76d8acdc9134549d80469ddd63de66ad35e
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c7a68fdb0183368932d909dd91e821cc52c1b9b2&newProject=try&newRevision=3f87f76d8acdc9134549d80469ddd63de66ad35e&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1
  DAMP reports no particular win.

DAMP complicated.netmonitor profiles:
* Without the patch
  https://perfht.ml/2D6xfNX
  waterfallbackground, draw = 230ms

* With the patch
  https://perfht.ml/2mk6UBp
  waterfallbackground, draw = 100ms

So 130ms win, still not enough to make DAMP report a significant win. But as draw method is called once per request, this win will quickly accumulate for big websites.
Attachment #8937556 - Flags: review?(jdescottes)
Attachment #8941876 - Flags: review?(odvarko)
Attachment #8941876 - Flags: review?(jdescottes)
Comment on attachment 8941876 [details]
Bug 1425964 - Prevent sync reflows by using document.dir to check for locale direction.

https://reviewboard.mozilla.org/r/212092/#review218118

LGTM.

Tools could query toolbox.direction rather than checking their document.dir. Or better, toolbox could expose a isRtl() method.
isRTL could be a prop passed to SplitBox.

::: devtools/client/shared/components/splitter/SplitBox.js:152
(Diff revision 1)
>      let { endPanelControl } = this.props;
>  
>      if (this.state.vert) {
>        // Switch the control flag in case of RTL. Note that RTL
>        // has impact on vertical splitter only.
> -      let dir = win.getComputedStyle(doc.documentElement).direction;
> +      if (document.dir == "rtl") {

You no longer need to define win here.
Attachment #8941876 - Flags: review?(jdescottes) → review+
Comment on attachment 8937556 [details]
Bug 1425964 - Compute locale direction only once in the toolbox code.

https://reviewboard.mozilla.org/r/208236/#review218122
Attachment #8937556 - Flags: review?(jdescottes) → review+
Comment on attachment 8937556 [details]
Bug 1425964 - Compute locale direction only once in the toolbox code.

https://reviewboard.mozilla.org/r/208236/#review218126

::: devtools/client/framework/toolbox.js:1758
(Diff revision 2)
> -      let topDocEl = top.document.documentElement;
> +    let topDocEl = top.document.documentElement;
> -      let isRtl = top.getComputedStyle(topDocEl).direction === "rtl";
> +    let isRtl = top.getComputedStyle(topDocEl).direction === "rtl";
> -      docEl.setAttribute("dir", isRtl ? "rtl" : "ltr");
> -    }
> +
> +    // getComputedStyle forces a synchronous reflow, so replace the getter in order to
> +    // call it only once.
> +    delete Toolbox.prototype.direction;

Actually, Toolbox's prototype is not reevaluated if we create a new toolbox. So if you change uidirection from 1 to -1 you need to restart the browser to see the changes in devtools. This should only be cached on the instance.
Attachment #8937556 - Flags: review+
Comment on attachment 8941876 [details]
Bug 1425964 - Prevent sync reflows by using document.dir to check for locale direction.

https://reviewboard.mozilla.org/r/212092/#review218588

Thanks for the patch!

R+ assuming try is green and my inline comments are resolved.

Honza

::: devtools/client/inspector/breadcrumbs.js:76
(Diff revision 1)
>  
>    /**
>     * Determine whether the current text directionality is RTL
>     */
>    isRtl: function () {
> -    return this.win.getComputedStyle(this.container).direction === "rtl";
> +    return this.doc.dir == "rtl";

Please keep `===` in the condition

::: devtools/client/shared/components/splitter/SplitBox.js:152
(Diff revision 1)
>      let { endPanelControl } = this.props;
>  
>      if (this.state.vert) {
>        // Switch the control flag in case of RTL. Note that RTL
>        // has impact on vertical splitter only.
> -      let dir = win.getComputedStyle(doc.documentElement).direction;
> +      if (document.dir == "rtl") {

I would prefere using `===`
Attachment #8941876 - Flags: review?(odvarko) → review+
(In reply to Julian Descottes [:jdescottes][:julian] from comment #12)
> Tools could query toolbox.direction rather than checking their document.dir.
> Or better, toolbox could expose a isRtl() method.
> isRTL could be a prop passed to SplitBox.

As we have to set document.dir for CSS rule ":dir(rtl)" to work, I find it great to also check against document.dir.
But yes, passing a prop may be more pure regarding React...

> Actually, Toolbox's prototype is not reevaluated if we create a new toolbox.
> So if you change uidirection from 1 to -1 you need to restart the browser to
> see the changes in devtools. This should only be cached on the instance.

Good catch, I'll change that.
Comment on attachment 8937556 [details]
Bug 1425964 - Compute locale direction only once in the toolbox code.

https://reviewboard.mozilla.org/r/208236/#review218772

Looks good and works fine, thanks! Just one comment to remove.

::: devtools/client/framework/toolbox.js:192
(Diff revision 3)
> +    // getComputedStyle forces a synchronous reflow, so replace the getter in order to
> +    // call it only once.

Comment no longer matching the code
Attachment #8937556 - Flags: review?(jdescottes) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0b830122fd3
Compute locale direction only once in the toolbox code. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ee28a6041e99
Prevent sync reflows by using document.dir to check for locale direction. r=Honza,jdescottes
https://hg.mozilla.org/mozilla-central/rev/a0b830122fd3
https://hg.mozilla.org/mozilla-central/rev/ee28a6041e99
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.