IonMonkey: Get to benchmark parity with post bug 804676 IonBuilder

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

Other Branch
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Right now bug 804676's patches (on the IonMonkey repo) regress octane/kraken by a couple %.  These regressions should be fixed before merging to trunk.
(Reporter)

Comment 1

5 years ago
Created attachment 737736 [details] [diff] [review]
Use baseline information for comparisons

Use baseline information to choose double specializations for comparisons where possible, in the absence of better type information.
Attachment #737736 - Flags: review?(jdemooij)
(Reporter)

Comment 2

5 years ago
Created attachment 737738 [details] [diff] [review]
avoid type barrier VM calls with type object guards

v8-deltablue hits some property writes where not all of the objects on the lhs have the property being written, but those other objects will not actually be seen at the site so will never have the property.  Rather than forcing a VM call here this patch adds a bailout testing the type of the object being written to.  This bailout should lead to an invalidation (via the property write itself happening) shortly afterwards.
Attachment #737738 - Flags: review?(dvander)
(Reporter)

Updated

5 years ago
Attachment #737738 - Attachment is patch: true
(Reporter)

Comment 3

5 years ago
Created attachment 737742 [details] [diff] [review]
fix definite properties bug

This fixes a performance bug where the definite properties analysis would ignore the last property added to 'this' if 'this' escaped later on in its constructor.  Landing this to IonMonkey even though it would also improve trunk, so I guess this is a thumb on the scale.  It seems to help richards a decent amount.
Attachment #737742 - Flags: review?(shu)
(Reporter)

Comment 4

5 years ago
Push with the above three patches.  On my machine these bring the 804676 stuff to parity with trunk on octane, or a little better.

https://hg.mozilla.org/projects/ionmonkey/rev/d746d516bf55
Comment on attachment 737736 [details] [diff] [review]
Use baseline information for comparisons

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

::: js/src/ion/MIR.cpp
@@ +1505,5 @@
> +    // instruction's type policy to insert fallible unboxes to the appropriate
> +    // input types.
> +
> +    if (!strictEq) {
> +        ICStub::Kind kind = inspector->monomorphicStubKind(pc);

What if we add a CompareICInspector class like SetElemICInspector, then this can be

if (inspector.isDoubleComparison()) {
    compareType_ = Compare_Double;
    return;
}
Attachment #737736 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #5)
> What if we add a CompareICInspector class like SetElemICInspector

Or maybe a method that returns the CompareType... As we are going to rely more on baseline caches it will be good to have a single way to access them I think.

Comment 7

5 years ago
Comment on attachment 737742 [details] [diff] [review]
fix definite properties bug

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

Nice catch! That bug must've been there for a while...
Attachment #737742 - Flags: review?(shu) → review+
(Reporter)

Comment 8

5 years ago
Created attachment 738804 [details] [diff] [review]
baseline comparison info, round 2

Encapsulate queries about comparisons better in the BaselineInspector class.  This applies on top of the previous patch and goes with the second alternative you proposed.  I am not a fan of the ICInspector as it is overengineered for its current use and going with this mechanism means a second one is needed when a query could be about multiple opcodes (maybeMonomorphicShapeForPropertyOp, expectedResultType).
Attachment #738804 - Flags: review?(jdemooij)

Updated

5 years ago
Attachment #738804 - Flags: review?(jdemooij) → review+
Comment on attachment 737738 [details] [diff] [review]
avoid type barrier VM calls with type object guards

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

This looks okay, but I'd prefer to see GuardShape/GuardType instead of GuardShapeOrType.
Attachment #737738 - Flags: review?(dvander)
(Reporter)

Comment 11

5 years ago
Created attachment 740783 [details] [diff] [review]
use GuardShape and GuardObjectType
Attachment #740783 - Flags: review?(dvander)
https://hg.mozilla.org/mozilla-central/rev/d746d516bf55
https://hg.mozilla.org/mozilla-central/rev/844053735e04
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 740783 [details] [diff] [review]
use GuardShape and GuardObjectType

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

Thanks!
Attachment #740783 - Flags: review?(dvander) → review+

Updated

5 years ago
Depends on: 865431

Updated

4 years ago
Depends on: 940635

Updated

4 years ago
Depends on: 973118
You need to log in before you can comment on or make changes to this bug.