Closed Bug 1357468 Opened 3 years ago Closed 3 years ago

Optimize Object.hasOwnProperty on Speedometer

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(2 files)

This patch was originally in bug 1344469, but I wanted to file a new bug to track this.

Basically with those patches we can cache all hasOwnProperty calls, but of course some of them are still megamorphic, so patches like in bug 1356315 will help.

This patch adds two additional features:
- Support for unboxed objects, which is a bit unclean by extending the native case, but this will be fixed soon.
- Dense elements, i.e code like [0].hasOwnProperty("0"). This happens often with for .. in loops.

This patch was reviewed by Jan in bug 1344469 comment 37.
Attachment #8859256 - Flags: review+
Oh sorry, I forgot about doing hasOwnProperty on Proxy objects, which is important for various DOM objects like NodeList etc.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d505fbfbd4e2
More Object.hasOwnProperty optimizations for Speedometer. r=jandem
Backed out for asserting an e.g. failing js/src/jit-test/tests/arrays/sort-getter-only.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/30e2910469f720c43bd913bb02fcb7ded4d19cc4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=89375918b96e056c339f8845c919361ac52bc91f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=92409199&repo=mozilla-inbound

[task 2017-04-18T17:06:37.604931Z] TEST-PASS | js/src/jit-test/tests/arrow-functions/church-1.js | Success (code 0, args "")
[task 2017-04-18T17:06:37.611541Z] Assertion failure: hasValue(), at /home/worker/workspace/build/src/js/src/jit/RegisterSets.h:210
[task 2017-04-18T17:06:37.611625Z] Exit code: -11
[task 2017-04-18T17:06:37.611682Z] FAIL - arrays/sort-getter-only.js
[task 2017-04-18T17:06:37.611857Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/arrays/sort-getter-only.js | Assertion failure: hasValue(), at /home/worker/workspace/build/src/js/src/jit/RegisterSets.h:210 (code -11, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2017-04-18T17:06:37.611904Z] INFO exit-status     : -11
[task 2017-04-18T17:06:37.611953Z] INFO timed-out       : False
[task 2017-04-18T17:06:37.612018Z] INFO stderr         2> Assertion failure: hasValue(), at /home/worker/workspace/build/src/js/src/jit/RegisterSets.h:210
[task 2017-04-18T17:06:37.619370Z] Assertion failure: hasValue(), at /home/worker/workspace/build/src/js/src/jit/RegisterSets.h:210
[task 2017-04-18T17:06:37.619455Z] Exit code: -11
Flags: needinfo?(evilpies)
Attached patch v2Splinter Review
In CacheIRCompiler::emitLoadDenseElementExistsResult we were using output.valueReg(), but in Ion this might be typed as boolean. So we have to use typedReg()
Flags: needinfo?(evilpies)
Attachment #8859359 - Flags: review?(jdemooij)
Comment on attachment 8859359 [details] [diff] [review]
v2

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

I only looked at the CacheIRCompiler.cpp changes.

We could rename LoadDenseElementExistsResult to GuardDenseElementExists and then emit LoadBooleanResult(true), but it doesn't really matter.

::: js/src/jit/CacheIRCompiler.cpp
@@ +1937,5 @@
> +    if (output.hasValue()) {
> +        masm.moveValue(BooleanValue(true), output.valueReg());
> +    } else {
> +        masm.movePtr(ImmWord(true), output.typedReg().gpr());
> +    }

Nit: no {}, or maybe add MOZ_ASSERT(output.type() == JSVAL_TYPE_BOOLEAN) to the else branch.
Attachment #8859359 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d555db01bb95
More Object.hasOwnProperty optimizations for Speedometer. r=jandem
https://hg.mozilla.org/mozilla-central/rev/d555db01bb95
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.