Unify Ion SETPROP and SETELEM IC stubs

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1209118 +++

Just like bug 1209118, but for SETPROP/SETELEM.

This is even nicer than that bug, because the SETELEM IC currently only handles indexed properties, while SETPROP has all the native/setter/proxy goodness, so we'll hopefully get that for SETELEM as well.
(Assignee)

Updated

4 years ago
Depends on: 1214163
(Assignee)

Updated

4 years ago
Depends on: 1214173
(Assignee)

Updated

4 years ago
Depends on: 1214562
(Assignee)

Comment 1

4 years ago
Pretty straight-forward.
Attachment #8674455 - Flags: review?(efaustbmo)
(Assignee)

Comment 2

4 years ago
Attachment #8674458 - Flags: review?(efaustbmo)
(Assignee)

Comment 3

4 years ago
I think we no longer need this hack. Let's see what the benchmarks do.
Attachment #8674461 - Flags: review?(efaustbmo)
(Assignee)

Comment 4

4 years ago
Moves the dense and typed array stubs to the SetPropertyIC.
Attachment #8674463 - Flags: review?(efaustbmo)
(Assignee)

Comment 5

4 years ago
10 files changed, 0 insertions(+), 314 deletions(-)
Attachment #8674464 - Flags: review?(efaustbmo)
(Assignee)

Comment 6

4 years ago
Using SETELEM to set/add string/symbol properties is way faster now:

before: 6084 ms
after:   276 ms

function f() {
    var sym = Symbol();
    var s = "foo";
    for (var i=0; i<10000000; i++) {
	var o = {};
	o[sym] = i;
	o[s] = 3;
    }
}
var t = new Date;
f();
print(new Date - t);
(Assignee)

Comment 7

4 years ago
13 files changed, 399 insertions(+), 520 deletions(-)
(Assignee)

Comment 8

4 years ago
8 day review ping ?
Comment on attachment 8674455 [details] [diff] [review]
Part 1 - Add extra operand

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

::: js/src/jit/IonCaches.cpp
@@ +3561,5 @@
>          cache.getScriptedLocation(&script, &pc);
>          MOZ_ASSERT(!script->hasNonSyntacticScope());
>          InitGlobalLexicalOperation(cx, &cx->global()->lexicalScope(), script, pc, value);
>      } else {
> +        RootedPropertyName name(cx, idval.toString()->asAtom().asPropertyName());

Ah, OK. This will do for now, but we will have to make this branching logic more complicated. I assume this happens in the coming patches. Maybe a MOZ_ASSERT there that this will work is customary, though it's certainly entailed in the accessors.
Attachment #8674455 - Flags: review?(efaustbmo) → review+
Comment on attachment 8674458 [details] [diff] [review]
Part 2 - Use MSetPropertyCache for SETELEM

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

There's a ton of guardHoles plumbing here, but it seems like it's never called in cache code? Should it have been?

::: js/src/jit/CodeGenerator.cpp
@@ +8478,5 @@
>  
>  void
>  CodeGenerator::addSetPropertyCache(LInstruction* ins, LiveRegisterSet liveRegs, Register objReg,
>                                     Register tempReg, ConstantOrRegister id, ConstantOrRegister value,
> +                                   bool strict, bool needsTypeBarrier, bool guardHoles,

We could consider to have a uint8_t for all these bools, but I don't care much.

::: js/src/jit/IonBuilder.cpp
@@ +9835,5 @@
>      current->add(ins);
>      current->push(value);
>  
> +    if (guardHoles)
> +        ins->setGuardHoles();

Why not put this in the constructor?

::: js/src/jit/IonCaches.cpp
@@ +1440,5 @@
> +    if (this->id().constant())
> +        return;
> +
> +    EmitIdGuard(masm, id, this->id().reg(), object(), temp(), fail);
> +}

Nice. This is a pleasant refactor in line with the current naming schemes. Thanks.

@@ +3270,5 @@
>  static void
>  GenerateSetUnboxed(JSContext* cx, MacroAssembler& masm, IonCache::StubAttacher& attacher,
>                     JSObject* obj, jsid id, uint32_t unboxedOffset, JSValueType unboxedType,
> +                   Register object, Register tempReg, ConstantOrRegister value, bool checkTypeset,
> +                   Label* failure)

can we keep this standard with everything else? I think |failures| is more customary when it's an argument
Attachment #8674458 - Flags: review?(efaustbmo) → review+
Comment on attachment 8674461 [details] [diff] [review]
Part 3 - Remove check

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

OK.
Attachment #8674461 - Flags: review?(efaustbmo) → review+
Comment on attachment 8674463 [details] [diff] [review]
Part 4 - Move stubs from SetElementIC to SetPropertyIC

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

::: js/src/jit/IonCaches.cpp
@@ +4401,4 @@
>      MacroAssembler masm(cx, ion, outerScript, profilerLeavePc_);
>      StubAttacher attacher(*this);
>      if (!GenerateSetDenseElement(cx, masm, attacher, obj, idval,
> +                                 guardHoles(), object(), id().reg(),

aha!

::: js/src/jit/shared/LIR-shared.h
@@ +5933,5 @@
>          setOperand(0, object);
>          setTemp(0, temp);
> +        setTemp(1, tempToUnboxIndex);
> +        setTemp(2, tempDouble);
> +        setTemp(3, tempFloat32);        

nit: trailing whitespace on tempFloat32 line.
Attachment #8674463 - Flags: review?(efaustbmo) → review+
Comment on attachment 8674464 [details] [diff] [review]
Part 5 - Remove SetElement IC

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

These are so cathartic.
Attachment #8674464 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 15

4 years ago
According to AWFY, this was a 31-33% win on Dromaeo's jslib-attr-prototype test :)

https://arewefastyet.com/#machine=17&view=single&suite=dromaeo&subtest=jslib-attr-prototype
Depends on: 1222905
Depends on: 1224895
Depends on: 1225367
Depends on: 1238935
(Assignee)

Updated

3 years ago
No longer depends on: 1238935
You need to log in before you can comment on or make changes to this bug.