Closed
Bug 1472132
Opened 6 years ago
Closed 6 years ago
|new Array| inlining in Ion needs to check new.target
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
The test below fails with --no-threads:
test.js:4:9 Error: Assertion failed: got [], expected ({})
--
function f() {
for (var i = 0; i < 20000; i++) {
var a = Reflect.construct(Array, [], Object);
assertEq(a.__proto__, Object.prototype);
}
}
f();
--
Adding this to IonBuilder::inlineArray fixes it:
if (callInfo.constructing() && callInfo.getNewTarget() != callInfo.fun())
return InliningStatus_NotInlined;
We probably want to add a check like that in the caller so it works for all natives we inline..
Comment 1•6 years ago
|
||
Similar issue when inlining String:
---
function f() {
for (var i = 0; i < 20000; i++) {
var a = Reflect.construct(String, [""], Object);
assertEq(a.__proto__, Object.prototype);
}
}
f();
---
And TypedArrays:
---
function f() {
for (var i = 0; i < 20000; i++) {
var a = Reflect.construct(Int32Array, [0], Object);
assertEq(a.__proto__, Object.prototype);
}
}
f();
---
Assignee | ||
Comment 2•6 years ago
|
||
I should probably just fix this (later today) instead of rewriting the test I wrote in the other bug..
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 3•6 years ago
|
||
I added the new.target != callee check to makeInliningDecision so it works for both native functions and inlining of Class hooks (inlineNonFunctionCall).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8989042 -
Flags: review?(andrebargull)
Comment 4•6 years ago
|
||
Comment on attachment 8989042 [details] [diff] [review]
Patch
Review of attachment 8989042 [details] [diff] [review]:
-----------------------------------------------------------------
I guess that works, but I'd expected that MCallOptimize rejects inlining when |new.target| doesn't match the expected value instead of making that decision at an earlier level. Well, if this approach turns out to be too coarse grained we can still change it in the future.
::: js/src/jit/IonBuilder.cpp
@@ +4004,5 @@
> + callInfo.getNewTarget() != callInfo.fun() &&
> + (!targetArg->is<JSFunction>() ||
> + !targetArg->as<JSFunction>().isInterpreted()))
> + {
> + return InliningDecision_DontInline;
Does it make sense to call |trackOptimizationOutcome| and/or |DontInline| here to track the inlining failure?
Attachment #8989042 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 5•6 years ago
|
||
How do you feel about moving the check to IonBuilder::inlineNativeCall and IonBuilder::inlineNonFunctionCall? I can do that if you prefer it.
Comment 6•6 years ago
|
||
That sounds good to me, because it makes it easier to spot when we reject to inline a native resp. non-function constructor call.
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #8989042 -
Attachment is obsolete: true
Attachment #8989196 -
Flags: review?(andrebargull)
Comment 8•6 years ago
|
||
Comment on attachment 8989196 [details] [diff] [review]
Patch v2
Review of attachment 8989196 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8989196 -
Flags: review?(andrebargull) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ca4ebfd22e
Don't inline non-scripted functions in Ion when constructing and new.target != the callee. r=anba
Comment 10•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•