Optimize Object.hasOwnProperty on Speedometer

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Created attachment 8859256 [details] [diff] [review]
Object.hasOwnProperty optimizations for Speedometer (unboxed objects and dense elements)

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+
(Assignee)

Comment 1

a year ago
Oh sorry, I forgot about doing hasOwnProperty on Proxy objects, which is important for various DOM objects like NodeList etc.

Comment 2

a year ago
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)
(Assignee)

Comment 4

a year ago
Created attachment 8859359 [details] [diff] [review]
v2

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+

Comment 7

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d555db01bb95
More Object.hasOwnProperty optimizations for Speedometer. r=jandem

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d555db01bb95
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.