Closed Bug 1453044 Opened 2 years ago Closed Last year

Resizing the window is very slow when Style Editor is enabled

Categories

(DevTools :: Style Editor, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

STRs:
- go to about:newtab (the issue is more visible on certain pages)
- open devtools
- open style editor
- resize the window (make it smaller / bigger a few times)

On my machine, the resize becomes sluggish very quickly.

This can be reproduced by running the following code in the Browser Console;

(function () {
  const MAX_ITERATIONS = 40;
  const MIN_WIDTH = 400;
  const MAX_WIDTH = 1500;
  const INTERVAL = 0;
  const STEP = 300;

  let sign = 1;
  let iterations = 0;

  performance.mark("START"); 
  top.resizeTo(700, 800); 
  interval = setInterval(()=>{
    performance.mark("RESIZE");
    top.resizeBy(sign * STEP, 0);

    if (top.outerWidth > MAX_WIDTH || top.outerWidth < MIN_WIDTH) {
      sign = -1 * sign;
      iterations++;
      if (iterations > MAX_ITERATIONS) {
        clearInterval(interval); 
        performance.mark("STOP");
      }
    }
  }, INTERVAL);
})();

And here is a performance profile recorded during a manual resize: 
https://perfht.ml/2IHFFdG
The issue is linked to source-maps and media queries.

If you have N media-queries impacted by a size change:
- each MediaRule will fire "matches-change" (so N events)
- for each "matches-change" the StyleSheetEditor.jsm responsible it will fire "media-rules-changed" (still N events)
- for each "media-rules-changed" StyleEditorUI.jsm will call _updateMediaList to redraw the media queries sidebar for this editor
- for each rule in the editor _updateMediaList will call getOriginalLocation using the sourcemap service (so N * N calls) which will spawn an async task, a worker etc...

This is the "good" scenario if only one original file contains all the N media queries. But if they are split amongst M original files, each StyleSheetEditor will have and watch all the mediaRules of the original file, even if it only displays its own part. They are filtered later on in _updateMediaList (by calling getOriginalLocation). Which means that in the end you get M * N * N calls to getOriginalLocation.

So it is very easy to go into very bad situations here, calling getOriginalLocation thousands of times for a simple resize. 

Simple improvement to do here: group "matches-change" events in StyleSheetEditor, before firing "media-rules-changed" (to go back to O(n) rather than O(n^2)). Another thing that should be straightforward: filter editor.mediaRules when building the StyleSheetEditor and keep only the rules relevant for this editor (done at the moment by consumers of editor.mediaRules).

There are also other leads to investigate:
- the style editor always renders all editors, even when not visible (but I doubt we want to invest on UI changes here)
- the sourcemapservice is repeatedly called with the exact same arguments, maybe we can introduce some caching
Here is a simple test case that should be easy to port to DAMP. Can't work on this right now, but if anyone feels like improving this there should be enough information on the bug to do so.

Goal would be to add a new (probably dedicated) DAMP test and implement one of the fixes suggested above.
Mentor: jdescottes
Some interesting pointers in case anyone wants to take this:

https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/devtools/client/styleeditor/StyleSheetEditor.jsm#378-399

This is where an editor starts to listen to media rule changes and where we fire the event caught by the UI. This is typically where we could add throttling before firing "media-rules-changed".

The second fix proposed was to filter the rules in StyleSheetEditor.jsm, this should also be done in the code mentioned above. this is also where we could filter the rules to only keep the ones that are really in this Editor. We can probably move the logic currently done in the UI: 

https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/devtools/client/styleeditor/StyleEditorUI.jsm#906-915

to StyleSheetEditor (and consequently remove it from the UI). This also means that we should store the original location for each rule somewhere in StyleSheetEditor to be able to send it to the UI. That's probably a bit more complex so it would be fine to try it in a second step.
Product: Firefox → DevTools
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
It seems that the new test, without the fix, crashes DAMP on try! https://treeherder.mozilla.org/#/jobs?repo=try&revision=520017211cdc204160780bb0d9b37147f5824a03

With the fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf05c76cfae636a863ac57d7f443ae76330986f1

Will try to see if I can get a version of the test which at least passes, to get a baseline.
This fixes a performance issue when a page has media-queries based
on the width of the viewport, and the user resizes the window.

If you have N media-queries impacted by a size change:
- each MediaRule will fire "matches-change" (so N events)
- for each "matches-change" the StyleSheetEditor.jsm responsible
  for it will fire "media-rules-changed" (still N events)
- for each "media-rules-changed" StyleEditorUI.jsm will call
  _updateMediaList to redraw the media queries sidebar for this editor
- for each rule in the editor _updateMediaList will call
  getOriginalLocation using the sourcemap service (so N * N calls)
  which will spawn an async task, a worker etc...

This is the "good" scenario if only one original file contains all
the N media queries. But if they are split amongst M original files,
each StyleSheetEditor will have and watch all the mediaRules of the
original file, even if it only displays its own part. They are
filtered later on in _updateMediaList (by calling getOriginalLocation).

Which means that in the end you get M * N * N calls to getOriginalLocation.

Throttling calls to media-rules-changed is an easy way to reduce the number
of actual calls to getOriginalLocation.
Attached image image.png
The test detects the expected improvement
Comparing with the base central: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=e521f87121e8&newProject=try&newRevision=85b1058c55408765ce4b8e97a3cd624c3f2579ce&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12

The compare seems to point at a side-effect on inspector.mutations. However the value of the central run seems like an exception, and current values on try/central for the same platform are consistent with the ones from the try run with the new test.

It looks like this test doesn't have real side effects on existing tests.
Attachment #9007420 - Attachment description: Bug 1453044 - Add new DAMP test for styleeditor resize with sourcemaps;r=ochameau → Bug 1453044 - Add new DAMP test for styleeditor resize with sourcemaps;r=nchevobbe
Comment on attachment 9007420 [details]
Bug 1453044 - Add new DAMP test for styleeditor resize with sourcemaps;r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9007420 - Flags: review+
Comment on attachment 9007414 [details]
Bug 1453044 - Throttle styleeditor mediaquery events;r=ochameau

Alexandre Poirot [:ochameau] PTO back on 17th has approved the revision.
Attachment #9007414 - Flags: review+
Comment on attachment 9009872 [details]
Bug 1453044 - Add mochitest to assert number of StyleEditor ui updates after resize;r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9009872 - Flags: review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e92a761d79bf
Add mochitest to assert number of StyleEditor ui updates after resize;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/73ac29c60e6d
Throttle styleeditor mediaquery events;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/e92a761d79bf
https://hg.mozilla.org/mozilla-central/rev/73ac29c60e6d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Attachment #9007420 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.