1373615 - Clean up and optimize property enumeration code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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:

before:
- unboxed: 255
- native:  161

after:
- 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);
}
f();
function g() {
    var t = new Date;
    for (var i = 0; i < 1000000; i++) {
	Object.keys(Object.prototype);
    }
    print(new Date - t);
}
g();

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8878441 [details] [diff] [review]
Patch

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!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c7dd6a9e935
Clean up and optimize property enumeration code. r=evilpie

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8c7dd6a9e935

Comment 5

[Jan de Mooij [:jandem]]

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):

https://arewefastyet.com/#machine=35&view=single&suite=speedometer-misc&subtest=Inferno-TodoMVC-CompletingAllItems-sync

A bit clearer on the Linux machine: https://arewefastyet.com/#machine=31&view=single&suite=speedometer-misc&subtest=Inferno-TodoMVC-CompletingAllItems-sync

Pretty sweet because there's definitely more we can optimize here.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Helped a lot of small benchmarks too, with small regressions which seem acceptable / in the range of noise: https://treeherder.mozilla.org/perf.html#/alerts?id=7394