Closed
Bug 1317703
Opened 8 years ago
Closed 8 years ago
Port Baseline native getter stub to CacheIR
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
h4writer
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Bug 1310125 did this for scripted getters (assuming it sticks), this bug is for native getters. Hopefully this will be much simpler as most of the infrastructure for call stubs etc is now in place.
Baseline's TryAttachNativeGetAccessorPropStub is very complicated and hard to understand atm, because it has to deal with getters and window proxies and DOM proxies. I hope we can disentangle this and structure this code more like the Ion IC, with separate paths for (DOM) proxies.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•8 years ago
|
||
* This doesn't convert the GetProp_CallNativeGlobal stub that's used for JSOP_GETNAME, once we convert that one we can remove more code.
* The old CallNative stub has some code to stash the object on the stack for the decompiler. This was added in bug 1153458 to avoid a decompiler assertion failure, but that assert has been turned into an if-statement so pushing for the decompiler is no longer necessary.
* I moved the "innerization" optimization to GetPropIRGenerator::tryAttachWindowProxy, to make it easier to understand than in the current code.
* I fixed a related issue in IonBuilder: if we innerize in IonBuilder, we only want to look for Baseline stubs that performed the same optimization. If we don't innerize in IonBuilder, we don't want to consider stubs that performed this optimization. This complicates the pattern matching a bit, but I confirmed it still works and it fixes a pre-existing issue.
Attachment #8811726 -
Flags: review?(hv1989)
Assignee | ||
Comment 2•8 years ago
|
||
bz, would you mind reviewing this patch too, especially the innerization/WindowProxy changes (see comment 1)? I don't think anyone else knows about this stuff.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8811726 [details] [diff] [review]
Patch
Review of attachment 8811726 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/SharedIC.cpp
@@ +2515,5 @@
> ICGetPropCallDOMProxyNativeCompiler compiler(cx, kind, info->engine(), monitorStub, proxy, holder,
> callee, info->pcOffset());
> newStub = compiler.getStub(compiler.getStubSpace(info->outerScript(cx)));
> } else {
> + return true;
FWIW, this code (and some other parts) can be simplified a bit now, but the rest of this code will (hopefully) also be removed soon, so I decided to leave it like this.
Comment 4•8 years ago
|
||
Comment on attachment 8811726 [details] [diff] [review]
Patch
The "innerized" arg to BaselineInspector::commonGetPropFunction could use some documentation in the header.
It might be good to document that tryAttachWindowProxy only really attaches (some kinds of?) JSNative getters where the receiver is the WindowProxy.
I think this all works, but I haven't really sorted out the details of the CacheIR stuff yet...
Flags: needinfo?(bzbarsky)
Attachment #8811726 -
Flags: review+
Comment 5•8 years ago
|
||
Comment on attachment 8811726 [details] [diff] [review]
Patch
Review of attachment 8811726 [details] [diff] [review]:
-----------------------------------------------------------------
Nice. So little code to allow native getters! I almost thought you only implemented the windows proxy stuff only.
Good stuff!
::: js/src/jit/BaselineCacheIR.cpp
@@ +995,5 @@
> case GuardClassKind::UnmappedArguments:
> clasp = &UnmappedArgumentsObject::class_;
> break;
> + case GuardClassKind::WindowProxy:
> + clasp = cx_->maybeWindowProxyClass();
Do we need to assert that clasp is non-empty?
@@ +1157,5 @@
> + // We use ICTailCallReg when entering the stub frame, so ensure it's not
> + // used for something else.
> + Maybe<AutoScratchRegister> tail;
> + if (allocator.isAllocatable(ICTailCallReg))
> + tail.emplace(allocator, masm, ICTailCallReg);
I think we should create "AutoEnterStubFrame" that does this.
I noticed this also in evilpie his patch and I think it would be more elegant with such a RAII.
::: js/src/jit/BaselineInspector.cpp
@@ +832,5 @@
> &stub->stubInfo()->getStubField<JSObject*>(stub, offset)->as<JSFunction>();
>
> + Shape* thisGlobalShape = nullptr;
> + if (getter->isNative() &&
> + receiver.shape &&
Style nit: Any reason this is not on the previous line. That won't make 80chars
Attachment #8811726 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #5)
> Do we need to assert that clasp is non-empty?
There's a MOZ_ASSERT(clasp) after the switch.
> I think we should create "AutoEnterStubFrame" that does this.
Agreed. I filed bug 1320118.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629069be312e
Port Baseline native getter stub to CacheIR. r=h4writer,bz
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•