Clean up and optimize property enumeration code

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Posted 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:

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();
Attachment #8878440 - Flags: review?(evilpies)
(Assignee)

Comment 1

2 years ago
Posted 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]
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!
Attachment #8878441 - Flags: review?(evilpies) → review+

Comment 3

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c7dd6a9e935
Clean up and optimize property enumeration code. r=evilpie
https://hg.mozilla.org/mozilla-central/rev/8c7dd6a9e935
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 5

2 years ago
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.
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
You need to log in before you can comment on or make changes to this bug.