Closed Bug 1396600 Opened 7 years ago Closed 7 years ago

Prevent loading properties-db in parent process, or delay its load

Categories

(DevTools :: Inspector, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

http://searchfox.org/mozilla-central/source/devtools/shared/css/properties-db.js
loads
http://searchfox.org/mozilla-central/source/devtools/shared/css/generated/properties-db.js
a 174K file

Today, we load this file in parent process for the frontend and also in the actors.
The copy loaded in the frontend shouldn't be loaded, ideally we would only use the one served from the actors, if possible.

While it is hard to untangle frontend code to always use the actor's version,
we can at least make some efforts to delay its load and prevent loading it at all in the parent process.

A profile without this patch:
https://perfht.ml/2ey2t2t
1.5ms in the parent process and 1.7ms in the child process

Another profile with this patch:
https://perfht.ml/2eyda56
5.9ms in the child process

The important fact is that it disappeared from the parent process.
It is reported as slower with this patch, but I think it is mosly related to the lazy loading in the child process. It is no longer loaded during actors load, but later, and I imagine there is more things happenning in parallel and reports slower loading.
Comment on attachment 8904281 [details]
Bug 1396600 - Make some efforts to lazy load properties-db.

https://reviewboard.mozilla.org/r/176054/#review181420

Thank you for the patch.  There's one nit but nothing serious.

> While it is hard to untangle frontend code to always use the actor's version, we can at least make some efforts to delay its load and prevent loading it at all in the parent process.

Ideally we would not need to transfer this at all, but would instead simply query the actor whenever information is needed.  However, we tried this last year and it proved to be very difficult.

::: devtools/client/inspector/inspector.js
(Diff revision 1)
>      // Localize all the nodes containing a data-localization attribute.
>      localizeMarkup(this.panelDoc);
>  
>      this._cssPropertiesLoaded = initCssProperties(this.toolbox);
>      yield this._cssPropertiesLoaded;
> -    yield this.target.makeRemote();

I don't understand the need for this change.

::: devtools/client/shared/css-angle.js:46
(Diff revision 1)
>    CssAngle: CssAngle,
>    classifyAngle: classifyAngle
>  };
>  
> -CssAngle.ANGLEUNIT = CSS_ANGLEUNIT;
> -
> +// Still keep trying to lazy load properties-db by lazily getting ANGLEUNIT
> +Object.defineProperty(CssAngle, "ANGLEUNIT", {

Seems like it would be simpler to just push this into the ".prototype = { ..." block just below.
Attachment #8904281 - Flags: review?(ttromey) → review+
(In reply to Tom Tromey :tromey from comment #2)
> Comment on attachment 8904281 [details]
> ::: devtools/client/inspector/inspector.js
> (Diff revision 1)
> >      // Localize all the nodes containing a data-localization attribute.
> >      localizeMarkup(this.panelDoc);
> >  
> >      this._cssPropertiesLoaded = initCssProperties(this.toolbox);
> >      yield this._cssPropertiesLoaded;
> > -    yield this.target.makeRemote();
> 
> I don't understand the need for this change.

This isn't related to css properties. I just saw this useless call.
This is redundant with:
http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#419-420

But I just realized this is done in every panel.
I'll do that cleanup in its own bug.

> ::: devtools/client/shared/css-angle.js:46
> (Diff revision 1)
> >    CssAngle: CssAngle,
> >    classifyAngle: classifyAngle
> >  };
> >  
> > -CssAngle.ANGLEUNIT = CSS_ANGLEUNIT;
> > -
> > +// Still keep trying to lazy load properties-db by lazily getting ANGLEUNIT
> > +Object.defineProperty(CssAngle, "ANGLEUNIT", {
> 
> Seems like it would be simpler to just push this into the ".prototype = {
> ..." block just below.

I wanted to keep this test as-is:
http://searchfox.org/mozilla-central/source/devtools/client/shared/test/browser_css_angle.js#51
but you are right, I'll change that.
Opened bug 1397020 for the makeRemote call removal.
Assignee: nobody → poirot.alex
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/492ea790f049
Make some efforts to lazy load properties-db. r=tromey
Blocks: 1397343
https://hg.mozilla.org/mozilla-central/rev/492ea790f049
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.