926725 - Optimize lazyGetters on Scopes

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect)

Description

[Brandon Benvie [:benvie]]

Right now there's lazyGetters defined for "_store", "_enumItems", "_nonEnumItems", and "_batchItems" in the Scope constructor. The accompanying comment says the purpose for this is to avoid instantiating these things for thousands of items unless needed. However, just creating the lazy getters themselves has to allocate a new function object which is probably worse than just creating the Map/Arrays. This can be improved by moving the lazyGetters to the prototype.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: bug-926725.patch

This patch adds `DevToolsUtils.defineLazyPrototypeGetter` which is a |this| sensitive version of XPCOMUtils.defineLazyGetter. It allows defining the lazy getter on the prototype such that it works with instances. This function is then used to define the various lazy getters on Scope.prototype.

Comment 2

[Brandon Benvie [:benvie]]

Attachment: bug-926725.patch

Add test file.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 816920 [details] [diff] [review]
bug-926725.patch

Review of attachment 816920 [details] [diff] [review]:
-----------------------------------------------------------------

See comment below, I honestly don't know if it is better or not.

::: toolkit/devtools/DevToolsUtils.js
@@ +148,5 @@
> + * @param Function aCallback
> + *        The callback that will be called to determine the value. Will be
> + *        called with the |this| value of the current instance.
> + */
> +this.defineLazyPrototypeGetter = 

Nit: trailing whitespace

@@ +159,5 @@
> +      Object.defineProperty(this, aKey, {
> +        configurable: true,
> +        writable: true,
> +        value: value
> +      });

Won't this force dictionary mode instead of sharing shapes? Is it better to check for the existence of `"_" + aKey` or close over the init'd value or something?

::: toolkit/devtools/DevToolsUtils.jsm
@@ +22,5 @@
>    safeErrorString: safeErrorString,
>    reportException: reportException,
>    makeInfallible: makeInfallible,
> +  yieldingEach: yieldingEach,
> +  defineLazyPrototypeGetter: defineLazyPrototypeGetter

God I hate this set up where you have to edit both files.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 816920 [details] [diff] [review]
bug-926725.patch

Review of attachment 816920 [details] [diff] [review]:
-----------------------------------------------------------------

What Nick said.

Comment 5

[Brandon Benvie [:benvie]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> @@ +159,5 @@
> > +      Object.defineProperty(this, aKey, {
> > +        configurable: true,
> > +        writable: true,
> > +        value: value
> > +      });
> 
> Won't this force dictionary mode instead of sharing shapes? Is it better to
> check for the existence of `"_" + aKey` or close over the init'd value or
> something?

Looks like redefining the property on the instance is significantly faster: http://jsperf.com/comparing-different-definelazyprototypegetter-methods


> >    safeErrorString: safeErrorString,
> >    reportException: reportException,
> >    makeInfallible: makeInfallible,
> > +  yieldingEach: yieldingEach,
> > +  defineLazyPrototypeGetter: defineLazyPrototypeGetter
> 
> God I hate this set up where you have to edit both files.

It might be possible to just do:
> this.DevToolsUtils = {};
> 
> Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>  .getService(Components.interfaces.mozIJSSubScriptLoader)
>  .loadSubScript("resource://gre/modules/devtools/DevToolsUtils.js",
>                 this.DevToolsUtils);

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> 
> Won't this force dictionary mode instead of sharing shapes? Is it better to
> check for the existence of `"_" + aKey` or close over the init'd value or
> something?

Does it make a difference if the properties are already available as null keys on the prototype? My understanding is that in that case the shape is unchanged if the lazy getter would look something like this:

_thing: null,
get actualThing() {
  if (this._thing == null) this._thing = new Map();
  return this._thing;
}

Maybe even the defineProperty call that changes a value property to a getter property would work in preserving the shape, but I have strong doubts.

Comment 7

[Brandon Benvie [:benvie]]

I just updated the jsperf to do exactly that and keeping the getter on the prototype is still almost 150x slower than just defining the property on the instance.

Comment 8

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=9c4c90c95136

Comment 9

[Victor Porof [:vporof][:vp]]

In this case it sounds like the same approach should be taken for the Item class and WidgetMethods widget setter method in ViewHelpers.jsm. What do you think? (Item is likely to be instantiated many times, while the WidgetMethods will be extended via Object.create() and the setter called only a handful of times at most).

Comment 10

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #9)
> In this case it sounds like the same approach should be taken for the Item
> class and WidgetMethods widget setter method in ViewHelpers.jsm. What do you
> think? (Item is likely to be instantiated many times, while the
> WidgetMethods will be extended via Object.create() and the setter called
> only a handful of times at most).

I agree. I could modify defineLazyPrototypeGetter to also have a setter which can be used to define the value. That way this is useful for both types of use cases.

Comment 11

[Brandon Benvie [:benvie]]

Actually nevermind on the setter, that's not needed. But yeah, there's a bunch of places in ViewHelpers.jsm that could be updated to use this. I've opened bug 927034 for that.

Comment 12

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/97f6836ede99

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97f6836ede99