Closed Bug 1373615 Opened 3 years ago Closed 3 years ago

Clean up and optimize property enumeration code


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox56 --- fixed


(Reporter: jandem, Assigned: jandem)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
This code is super hot and yet it's much slower than necessary:

* When we are enumerating own properties and have a newEnumerate hook, we get paranoid and always check for duplicates. This is unnecessary for unboxed objects because we know these hooks don't return duplicates. This shows up quite a lot on Speedometer.

* In Enumerate (called for each property we see), we decide if we need to check for duplicates. It's faster (and simpler even) to hoist this check all the way up into Snapshot.

Initially I passed this as bool argument but then I decided to make it a CheckForDuplicates template parameter so the compiler can inline Enumerate into the callers and generate much better code.

I also had to factor out the proxy enumeration code into EnumerateProxyProperties so we can templatize it.

Below is a simple micro-benchmark for both unboxed object enumeration and native objects. I get the following results:

- unboxed: 255
- native:  161

- unboxed: 117
- native:  121

function f() {
    var t = new Date;
    for (var i = 0; i < 1000000; i++)
	Object.getOwnPropertyNames({x: 1, y: 2, z: 3});
    print(new Date - t);
function g() {
    var t = new Date;
    for (var i = 0; i < 1000000; i++) {
    print(new Date - t);
Attachment #8878440 - Flags: review?(evilpies)
Attached patch PatchSplinter Review
Attachment #8878440 - Attachment is obsolete: true
Attachment #8878440 - Flags: review?(evilpies)
Attachment #8878441 - Flags: review?(evilpies)
Comment on attachment 8878441 [details] [diff] [review]

Review of attachment 8878441 [details] [diff] [review]:

\o/ Great per results and it does read nicer.

::: js/src/jsiter.cpp
@@ +300,5 @@
> +}
> +
> +template <bool CheckForDuplicates>
> +static bool
> +EnumerateProxyProperties(JSContext* cx, HandleObject pobj, unsigned flags, Maybe<IdSet>& ht,

+1 for factoring this out.

@@ +448,5 @@
>                  }
>              } else {
> +                // The newEnumerate hook may return duplicates. Whitelist the
> +                // unboxed object hooks because we know they are well-behaved.
> +                if (!pobj->is<UnboxedPlainObject>() && !pobj->is<UnboxedArrayObject>())

Great idea!
Attachment #8878441 - Flags: review?(evilpies) → review+
Pushed by
Clean up and optimize property enumeration code. r=evilpie
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Needs more data points but looks like this was a measurable win on Inferno CompletingAllItems (where I noticed the unboxed object issue in comment 0):

A bit clearer on the Linux machine:

Pretty sweet because there's definitely more we can optimize here.
Helped a lot of small benchmarks too, with small regressions which seem acceptable / in the range of noise:
You need to log in before you can comment on or make changes to this bug.