Create an IC for proxy set()

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: efaust)

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

This is much like bug 649887 but for setelem or setprop.
(Reporter)

Updated

6 years ago
Blocks: 802157
(Reporter)

Updated

5 years ago
Depends on: 875452
(Reporter)

Updated

5 years ago
Depends on: 901100
(Assignee)

Updated

5 years ago
Blocks: 785467
No longer blocks: 802157
(Assignee)

Updated

5 years ago
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 792664 [details] [diff] [review]
Part -1: Fix frightening SetProperty() call

In pursuing proxy perfection, we need to make the SetProp ICs accessible in places where TI is unsure about the incoming types (since proxies don't keep TI, this shouldn't cause extra invalidations). In working on that (patch to come shortly), I discovered that our SetProperty ICs were broken! Here's a fix :)
Attachment #792664 - Flags: review?(jdemooij)
(Assignee)

Comment 2

5 years ago
Comment on attachment 792664 [details] [diff] [review]
Part -1: Fix frightening SetProperty() call

Canceling review. This was found and fixed independently in bug 906284.
Attachment #792664 - Flags: review?(jdemooij)
(Assignee)

Comment 3

5 years ago
looking fairly happy on try: https://tbpl.mozilla.org/?tree=Try&rev=5a73ea8e5e9e

patches up in the morning
(Assignee)

Comment 4

5 years ago
Created attachment 793007 [details] [diff] [review]
Part 0: Open SetPropertyIC to cases with uncertain TI.

In order to get proxies into the ICs, we need to teach them how to deal with objects with less-than-completely understood objects flowing in.
Attachment #792664 - Attachment is obsolete: true
Attachment #793007 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

5 years ago
Created attachment 793009 [details] [diff] [review]
Part 1: Rename ProxyGetExitFrame to ProxyExitFrame

Both proxy::get and proxy::set have the same marking profile, so we can reuse the fake exit frame.
Attachment #793009 - Flags: review?(kvijayan)
(Assignee)

Comment 6

5 years ago
Created attachment 793010 [details] [diff] [review]
Part 2: Implement generic Proxy::set stub for SetPropertyIC
Attachment #793010 - Flags: review?(kvijayan)

Updated

5 years ago
Attachment #793009 - Flags: review?(kvijayan) → review+
Comment on attachment 793007 [details] [diff] [review]
Part 0: Open SetPropertyIC to cases with uncertain TI.

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

::: js/src/jit/IonCaches.cpp
@@ +1948,5 @@
>  
> +    Label failures;
> +    masm.branchPtr(Assembler::NotEqual,
> +                   Address(object(), JSObject::offsetOfShape()),
> +                   ImmGCPtr(obj->lastProperty()), &failures);

In the needsTypeBarrier() case, this needs a type test as well as a shape test.  Two objects with the same shape can have different types.

@@ +1950,5 @@
> +    masm.branchPtr(Assembler::NotEqual,
> +                   Address(object(), JSObject::offsetOfShape()),
> +                   ImmGCPtr(obj->lastProperty()), &failures);
> +
> +    // No need to emit if we can know statically.

This comment doesn't make much sense.  'Guard that the incoming value is in the type set for the property, if a type barrier is required.'

@@ +1975,5 @@
> +                                      &barrierSuccess, &barrierFailure);
> +                } else {
> +                    masm.guardTypeSet(valReg, propTypes, scratchReg,
> +                                      &barrierSuccess, &barrierFailure);
> +                }

This |if| statement looks pretty strange, what keeps you from coalescing the two cases?  Can you add a comment, or make some more changes to let the generic TypedOrValueRegister case work?

@@ +2343,5 @@
>          return false;
>  
> +    types::TypeObject *type = obj->getType(cx);
> +    if (cache.needsTypeBarrier() && !type->unknownProperties())
> +    {

Nit: { on the previous line.

::: js/src/jit/LIR-Common.h
@@ +4133,5 @@
>  // Patchable jump to stubs generated for a SetProperty cache, which stores a
>  // boxed value.
>  class LSetPropertyCacheV : public LInstructionHelper<0, 1 + BOX_PIECES, 1>
>  {
> +    bool needsTypeBarrier_;

This flag can just be obtained from the MIR node instead.

@@ +4160,5 @@
>  // value of a known type.
>  class LSetPropertyCacheT : public LInstructionHelper<0, 2, 1>
>  {
>      MIRType valueType_;
> +    bool needsTypeBarrier_;

Ditto.
Attachment #793007 - Flags: review?(bhackett1024) → review+
Comment on attachment 793010 [details] [diff] [review]
Part 2: Implement generic Proxy::set stub for SetPropertyIC

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

Have you measured cases where these stubs get generated?  Is there anyplace where this generic stub gets generated, but there are also a large number of passes which are more heavily optimizable that might get passed over because of the presence of this stub?

Outside of that concern, patch logic looks good.

::: js/src/jit/IonCaches.cpp
@@ +2067,5 @@
>      return true;
>  }
>  
> +static bool
> +EmitCallProxySet(JSContext *cx, MacroAssembler &masm, IonCache::StubAttacher &attacher,

The previous codegen helpers are all named |Generate...|.  This one is named |Emit...|.  Might be good to call this one |GenerateCallProxySet| as well.  Especially since the Emit naming convention is more used by baseline code.

@@ +2163,5 @@
> +
> +        Register scratch = regSet.takeGeneral();
> +        masm.push(scratch);
> +
> +        GenerateProxyClassGuards(masm, object(), scratch, &proxyFailures, &proxySuccess);

Is there any way to expand these guards to be more than just "this is generically optimizable", and into "this is generically optimizable, and we're pretty sure there's no better optimized stubs we can create for this object."?
Attachment #793010 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/mozilla-central/rev/c1ccfd8f31bf
https://hg.mozilla.org/mozilla-central/rev/7180f4ffb7f1
https://hg.mozilla.org/mozilla-central/rev/61ac97a7fc7c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 911707
Depends on: 911708
Depends on: 913716

Updated

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