Optimize lazyGetters on Scopes

RESOLVED FIXED in Firefox 27

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: bbenvie, Assigned: bbenvie)

Tracking

(Blocks 1 bug)

Trunk
Firefox 27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch bug-926725.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Add test file.
Attachment #816916 - Attachment is obsolete: true
Attachment #816920 - Flags: review?(vporof)
Attachment #816920 - Flags: review?(nfitzgerald)
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.
Attachment #816920 - Flags: review?(nfitzgerald) → review+
Comment on attachment 816920 [details] [diff] [review]
bug-926725.patch

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

What Nick said.
Attachment #816920 - Flags: review?(vporof) → review+
(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);
(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.
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.
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).
Blocks: 927034
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/97f6836ede99
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.