Port Baseline GetName IC to CacheIR

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine: JIT
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 months ago
Created attachment 8820420 [details] [diff] [review]
Port Baseline GetName_Env to CacheIR

I think this is relatively straightforward. I had to change some code around input operands in BaselineCacheIR.cpp, because GetName gets an unboxed object on the stack. This -50 overall lines, but porting the other two ICs should make this a bigger win.
Attachment #8820420 - Flags: review?(jdemooij)
(Assignee)

Comment 2

4 months ago
Created attachment 8820452 [details] [diff] [review]
WIP Port GlobalNameValue

I still need to check this over a bit more, ICGetPropNativeCompiler is really not obvious.

This patch allows us to remove ICGetPropNativeStub completely, but there might be even more stuff we can remove now that I missed.

7 files changed, 82 insertions(+), 407 deletions(-)
(Assignee)

Comment 3

4 months ago
Mostly I am curious if we need to guard on the group or shape of objects on the prototype chain. For GetProp we usually only do the proto guard if we have an UncacheableProto. Presumably this is not possible here?
(In reply to Tom Schuster [:evilpie] from comment #3)
> Mostly I am curious if we need to guard on the group or shape of objects on
> the prototype chain. For GetProp we usually only do the proto guard if we
> have an UncacheableProto. Presumably this is not possible here?

__proto__ mutation may trigger hasUncacheableProto, but these objects are singletons so you may have to put a non-singleton on the proto chain first. However, this is the global right? The global has an immutable proto chain so we can probably just assert that?
Comment on attachment 8820420 [details] [diff] [review]
Port Baseline GetName_Env to CacheIR

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

::: js/src/jit/CacheIR.h
@@ +744,4 @@
>      CacheKind cacheKind() const { return cacheKind_; }
>  };
>  
> +// GetPropIRGenerator generates CacheIR for a GerName IC.

GetName
(In reply to Jan de Mooij [:jandem] from comment #4)
> However, this is the global right? The global has an immutable proto chain
> so we can probably just assert that?

This is true in the browser embedding, but SpiderMonkey doesn't enforce this requirement on all global objects.  Yet.  I have some patchwork locally to do this, but that patch possibly should wait for a full rethink of global-object creation that would specify the entire prototype chain at the same time (versus the current thing where people must manually JS_SplicePrototype their desired prototype into the chain).

In any event, right now we can't just assert this.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> This is true in the browser embedding, but SpiderMonkey doesn't enforce this
> requirement on all global objects.  Yet.

OK, thanks. Then we should probably check hasUncacheableProto and not attach if that's true - it means someone is messing with the global's prototype chain and if that can't even happen in the browser, I don't think we need to support this.
(Assignee)

Comment 8

4 months ago
Comment on attachment 8820420 [details] [diff] [review]
Port Baseline GetName_Env to CacheIR

Resetting review for now. This needs to rebased, which removes some ugly code. And the shapes.append call can fail, so I need to avoid that somehow.
Attachment #8820420 - Flags: review?(jdemooij)
(Assignee)

Comment 9

4 months ago
Created attachment 8820899 [details] [diff] [review]
v2 - Port Baseline GetName_Env to CacheIR
Attachment #8820420 - Attachment is obsolete: true
Attachment #8820899 - Flags: review?(jdemooij)
Comment on attachment 8820899 [details] [diff] [review]
v2 - Port Baseline GetName_Env to CacheIR

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

Looks great! Some suggestions below, sorry for the rebase pain.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1119,4 @@
>  }
>  
>  bool
> +BaselineCacheIRCompiler::emitLoadEnclosingEnv()

If you rebase this again (sorry this should be the last time..) we can share this op with Ion.

@@ +1197,5 @@
>  }
>  
>  bool
> +BaselineCacheIRCompiler::emitLoadEnvNameFixedSlotResult()
> +{

And use AutoOutputRegister + AutoScratchRegisterMaybeOutput here

@@ +1209,5 @@
> +    masm.load32(stubAddress(reader.stubOffset()), scratch);
> +
> +    // Check for uninitialized lexicals.
> +    BaseIndex slot(obj, scratch, TimesOne);
> +    masm.branchTestMagic(Assembler::Equal, slot, failure->label());

With AutoOutputRegister, I *think* we can now use Load{Fixed,Dynamic}SlotResult and do the branchTestMagic as a separate fallible op, without clobbering |obj| anywhere (the allocator should move it from R0 to another register). What do you think? It would be similar to the CrossCompartmentWrapper thing.

@@ -1221,4 @@
>      // unlimited number of stubs.
>      MOZ_ASSERT(stub->numOptimizedStubs() < MaxOptimizedCacheIRStubs);
>  
> -    MOZ_ASSERT(kind == CacheKind::GetProp || kind == CacheKind::GetElem);

Let's keep this assert for now: when we add SetProp we need to update this code to not use sizeof(ICCacheIR_Monitored).

::: js/src/jit/CacheIR.h
@@ +755,5 @@
> +    jsbytecode* pc_;
> +    HandleObject env_;
> +    HandlePropertyName name_;
> +    ICStubEngine engine_;
> +    CacheKind cacheKind_;

Nit: I think we should add an IRGenerator base class and move most of these members (and writerRef/cacheKind) into it. Later we can then also add (shared) emit* methods to the base class, like emitIdGuard.

I also think we should have ICStubEngine only in GetPropIRGenerator: ideally the IR will be independent of the IC kind. For GetProp we need it for now for scripted getters that we don't support in Ion shared-stubs but if/when we remove shared stubs we can hopefully get rid of it.

@@ +758,5 @@
> +    ICStubEngine engine_;
> +    CacheKind cacheKind_;
> +
> +    GetNameIRGenerator(const GetNameIRGenerator&) = delete;
> +    GetNameIRGenerator& operator=(const GetNameIRGenerator&) = delete;

If we do this in IRGenerator we no longer need it in derived classes AFAIK.

::: js/src/jit/SharedIC.cpp
@@ -373,5 @@
> -      case ICStub::GetName_Env5:
> -        static_cast<ICGetName_Env<5>*>(this)->traceEnvironments(trc);
> -        break;
> -      case ICStub::GetName_Env6:
> -        static_cast<ICGetName_Env<6>*>(this)->traceEnvironments(trc);

Variable number of shapes was a real pain, glad we can get rid of all this boilerplate \o/
Attachment #8820899 - Flags: review?(jdemooij)
(Assignee)

Comment 11

4 months ago
Created attachment 8821514 [details] [diff] [review]
v3 - Port Baseline GetName_Env to CacheIR

I don't think sharing the Load[Fixed]Slot code is going to be very nice. We currently don't have a good way to use the result as input to another instruction. Just adding this special case for two Ops doesn't seem worth it. However seeing as we probably will need this for CrossCompartmentWrapper (bug 1319087) as well, I am going to look into how to do this. At that point we can probably also move this out of BaselineCacheIRCompiler
Attachment #8820899 - Attachment is obsolete: true
Attachment #8821514 - Flags: review?(jdemooij)
Comment on attachment 8821514 [details] [diff] [review]
v3 - Port Baseline GetName_Env to CacheIR

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

Beautiful.
Attachment #8821514 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 13

4 months ago
Created attachment 8821524 [details] [diff] [review]
Port Baseline GlobalNameValue to CacheIR

Added the hasImmutablePrototype check and optimized the global/holder check to omit the unnecessary load when global == holder.
Attachment #8820452 - Attachment is obsolete: true
Attachment #8821524 - Flags: review?(jdemooij)
(Assignee)

Comment 14

4 months ago
I think we might be running out of registers on x86 ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92d952a993b9298f945a6cb5969a774ac81a8cc&selectedJob=33396400

Comment 15

4 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f75fcf25c4a0
Set available registers after initializing the input locations. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/094c2806ef37
Port Baseline GetName_Env to CacheIR. r=jandem
(Assignee)

Comment 16

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a33cd6b281f834217875eac8d7147422847b3d15

Jan provided the first patch over IRC yesterday, I just reviewed it myself. The actual patch had a small change to explicitly give back R0.typeReg() on x86 32bit, because ICStubCompiler::availableGeneralRegs assumes that for one input the R0 payload and type registers are used.

Comment 17

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f75fcf25c4a0
https://hg.mozilla.org/mozilla-central/rev/094c2806ef37
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

4 months ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Comment on attachment 8821524 [details] [diff] [review]
Port Baseline GlobalNameValue to CacheIR

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

Great that we don't need any IR/codegen changes for this, and that the "which guards do we emit" logic is now in a single place.

::: js/src/jit/CacheIR.cpp
@@ +1197,5 @@
> +        size_t dynamicSlotOffset = current->dynamicSlotIndex(shape->slot()) * sizeof(Value);
> +        writer.loadDynamicSlotResult(objId, dynamicSlotOffset);
> +    } else {
> +        // Check the prototype chain from the global to the current
> +        // prototype. Ignore the global lexical scope as it doesn' figure

Pre-existing nit: s/doesn'/doesn't/

@@ +1207,5 @@
> +        // Shape guard for global lexical.
> +        writer.guardShape(objId, globalLexical->lastProperty());
> +
> +        // If we are generating a non-lexical GETGNAME stub, we must also
> +        // guard on the shape of the GlobalObject.

Nit: we can remove the first part of this comment, as this is always a non-lexical GETGNAME. The old code had it because it used to be shared with GETPROP stubs.

::: js/src/jit/SharedIC.h
@@ -2464,5 @@
> -        return static_cast<int32_t>(engine_) |
> -              (static_cast<int32_t>(kind) << 1) |
> -              (static_cast<int32_t>(isFixedSlot_) << 17) |
> -              (static_cast<int32_t>(inputDefinitelyObject_) << 18) |
> -              (HeapReceiverGuard::keyBits(obj_) << 19);

Not going to miss this.
Attachment #8821524 - Flags: review?(jdemooij) → review+

Comment 19

4 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4398ee2eea0c
Port Baseline GlobalNameValue to CacheIR. r=jandem

Comment 20

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4398ee2eea0c
Should this now get closed?
Priority: -- → P3
(Assignee)

Comment 22

4 months ago
No, there is still one IC missing.
(Assignee)

Comment 23

4 months ago
Created attachment 8822031 [details] [diff] [review]
WIP  Baseline GlobalNameAccessor to CacheIR
(Assignee)

Comment 24

4 months ago
IsCacheableGetPropCallNative returns false for IsWindow, which is going to be most cases here, not sure if that is a problem? I ran this in the browser and some seems to have that special JIT info that makes it okay to attach.
(Assignee)

Comment 25

4 months ago
Created attachment 8822041 [details] [diff] [review]
Port Baseline GlobalNameAccessor to CacheIR

I think my patch is correct in that regard.
Stats:
11 files changed, 163 insertions(+), 604 deletions(-)
There is probably more stuff related to GetProp that can be removed now, but I didn't check that hard.
Attachment #8822031 - Attachment is obsolete: true
Attachment #8822041 - Flags: review?(jdemooij)
Comment on attachment 8822041 [details] [diff] [review]
Port Baseline GlobalNameAccessor to CacheIR

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

::: js/src/jit/BaselineIC.cpp
@@ -2510,5 @@
>          attached = true;
>      }
>  
> -    if (!attached && IsGlobalOp(JSOp(*pc)) && !script->hasNonSyntacticScope()) {
> -        if (!TryAttachGlobalNameAccessorStub(cx, script, pc, stub,

We should move the CacheIR IC code here (before the GetEnvironmentName call). Some getters add properties and trigger a shape change, in that case our shape guards always fail if we attach the stub after the fallback code. For GetName that's less relevant, but we have seen it with GetProp.

::: js/src/jit/BaselineInspector.cpp
@@ -904,1 @@
>              if (!AddCacheIRGetPropFunction(stub->toCacheIR_Monitored(), innerized,

We have to fix AddCacheIRGetPropFunction to match the extra shape check we add for the global lexical scope. Can probably skip LoadEnclosingEnvironment + GuardShape if we detect these, just need to make sure we get the right receiver/holder shapes.

Please make sure the getPropTryCommonGetter optimization in IonBuilder::jsop_getgname still works as before for own and inherited properties.
(Assignee)

Comment 27

4 months ago
Created attachment 8822092 [details] [diff] [review]
Inspector support for GlobalNameGetter

There are all kinds of weird conditions in this code that aren't exactly obvious, but I did verify that the optimization works for native getters directly on the global and on the prototype chain.
(Assignee)

Comment 28

4 months ago
I just noticed I don't guard on the global lexical at all in the Inspector, so something is probably wrong.
Depends on: 1326152
(Assignee)

Comment 29

4 months ago
Created attachment 8822405 [details] [diff] [review]
v2 -  Inspector support for GlobalNameGetter

I think this version of the patch should match the previous behavior.
Attachment #8822092 - Attachment is obsolete: true
(Assignee)

Comment 30

4 months ago
Created attachment 8822471 [details] [diff] [review]
v2 - Port Baseline GlobalNameAccessor to CacheIR
Attachment #8822041 - Attachment is obsolete: true
Attachment #8822041 - Flags: review?(jdemooij)
Attachment #8822471 - Flags: review?(jdemooij)
(Assignee)

Updated

4 months ago
Attachment #8822405 - Attachment is obsolete: true
(Assignee)

Comment 31

4 months ago
Created attachment 8822473 [details] [diff] [review]
v2 -  Inspector support for GlobalNameGetter

If we can't use script->global() we can just keep baking in holder even when it's the global.
Attachment #8822473 - Flags: review?(jdemooij)
Comment on attachment 8822471 [details] [diff] [review]
v2 - Port Baseline GlobalNameAccessor to CacheIR

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

Excellent.
Attachment #8822471 - Flags: review?(jdemooij) → review+
Comment on attachment 8822473 [details] [diff] [review]
v2 -  Inspector support for GlobalNameGetter

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

Yeah, this pattern matching is annoying. Hopefully it's just a stopgap until bug 1324561 replaces it.

::: js/src/jit/BaselineInspector.cpp
@@ +731,5 @@
> +    //   GuardShape objId
> +    //   globalId = LoadEnclosingEnvironment objId
> +    //   GuardShape globalId
> +    //   <holderId = LoadObject <holder>>
> +    //   <GuardShape holderId>

Nit: add "CallNativeGetterResult globalId"

@@ +746,5 @@
> +    ObjOperandId globalId = reader.objOperandId();
> +
> +    if (!reader.matchOp(CacheOp::GuardShape, globalId))
> +        return false;
> +    Shape* globalShape = stub->stubInfo()->getStubField<Shape*>(stub, reader.stubOffset());

Nit: maybe add:

MOZ_ASSERT(globalShape->getObjectClass()->flags & JSCLASS_IS_GLOBAL);

@@ +748,5 @@
> +    if (!reader.matchOp(CacheOp::GuardShape, globalId))
> +        return false;
> +    Shape* globalShape = stub->stubInfo()->getStubField<Shape*>(stub, reader.stubOffset());
> +
> +    JSObject* holder = &script->global(); // Todo: Not sure if we want to do this.

I think this is OK. The holder must be the (same) global object.

@@ +758,5 @@
> +        if (!reader.matchOp(CacheOp::GuardShape, holderId))
> +            return false;
> +        holderShape = stub->stubInfo()->getStubField<Shape*>(stub, reader.stubOffset());
> +    }
> +

At this point we can check

  if (holder->as<NativeObject>().lastProperty() != holderShape)
      return true;

|true| to skip this stub and try the next one, as this one will always fail. We used to update Baseline stubs that were outdated, but as we no longer do that we should check this in the inspector.
Attachment #8822473 - Flags: review?(jdemooij) → review+
Comment on attachment 8822473 [details] [diff] [review]
v2 -  Inspector support for GlobalNameGetter

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

::: js/src/jit/BaselineInspector.cpp
@@ +776,5 @@
> +        *holderShape_ = holderShape;
> +        *commonGetter = getter;
> +        *globalShape_ = globalShape;
> +
> +        // This is always false, because the getters never live on the globalLexial.

nit: globalLexical
(Assignee)

Updated

4 months ago
Keywords: leave-open

Comment 35

4 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1832a6e47f1c
Port Baseline GlobalNameAccessor to CacheIR. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85df7c92997
Inspector support for GlobalNameGetter. r=jandem
(Assignee)

Updated

4 months ago
Blocks: 1326437

Comment 36

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1832a6e47f1c
https://hg.mozilla.org/mozilla-central/rev/f85df7c92997
Status: REOPENED → RESOLVED
Last Resolved: 4 months ago4 months ago
Resolution: --- → FIXED
Depends on: 1326589
Blocks: 1326152
No longer depends on: 1326152
You need to log in before you can comment on or make changes to this bug.