Closed Bug 1357468 Opened 4 years ago Closed 4 years ago

Optimize Object.hasOwnProperty on Speedometer


(Core :: JavaScript Engine: JIT, enhancement)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: evilpie, Assigned: evilpie)




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

Push with failures:
Failure log:

[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]

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
More Object.hasOwnProperty optimizations for Speedometer. r=jandem
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.