Closed
Bug 1383643
Opened 7 years ago
Closed 4 years ago
IsPackedArray not inlined when Array.prototype.concat is called with non-arrays
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
DUPLICATE
of bug 1652685
People
(Reporter: anba, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
Test case:
---
(function() {
for (var i = 0; i < 10000; ++i) {
[].concat({}, {});
}
})();
---
If we break here [1] with |br MCallOptimize.cpp:1769 if clasp==0|, and then inspect the result-type set |p array->resultTypeSet_->getObjectCount()| resp. |p {array->resultTypeSet_->getObjectClass(0), array->resultTypeSet_->getObjectClass(1), array->resultTypeSet_->getObjectClass(2)}|, we see that the two object literals are included in the result-type set even though IsPackedArray isn't actually called with the objects. (IsPackedArray is only called for "concat-spreadable" objects [2].)
The same issue is also reproducible in Speedometer, which may explain the high call-count in bug 1365361 (third most called intrinsic).
I've also observed that IsPackedArray isn't inlined in React-TodoMVC and React-Redux-TodoMVC, because getInlineReturnType() returns MIRType::Value. I'm not sure if that's related to this issue or another problem.
[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/jit/MCallOptimize.cpp#1769
[2] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/builtin/Array.js#1045,1053
Comment 1•7 years ago
|
||
We should probably use types->forAllClasses(IsArray) here to optimize the ALL_FALSE case to a constant false.
I assume the the MIRType::Value return can happen when we never executed that function? Might be related if we never concat with a real array.
Comment 2•7 years ago
|
||
We must have bad type information here, because we never get ALL_FALSE.
Comment 3•7 years ago
|
||
I also checked some real world websites, we never get ALL_FALSE. And MIXED almost always ends up not working, because the object flags don't check out.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #3)
> And MIXED almost always ends up not working, because the object flags don't check out.
Not sure if it matters, but I think I added to many flags in inlineIsPackedArray(). The IsPackedArray() function in NativeObject-inl.h only checks for OBJECT_FLAG_NON_PACKED...
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to André Bargull from comment #4)
> (In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from
> comment #3)
> > And MIXED almost always ends up not working, because the object flags don't check out.
>
> Not sure if it matters, but I think I added to many flags in
> inlineIsPackedArray(). The IsPackedArray() function in NativeObject-inl.h
> only checks for OBJECT_FLAG_NON_PACKED...
I've tested this and even only checking for OBJECT_FLAG_NON_PACKED won't help: We always end up with TypeSet::unknownObject() returning |true|, because the TYPE_FLAG_ANYOBJECT flag is set.
Comment 6•7 years ago
|
||
We should probably just support checking those flags from JIT code as well.
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Assignee: nobody → evilpies
Comment 7•5 years ago
|
||
The code here hasn't changed, but I am wondering if we still want to do this?
Comment 9•4 years ago
|
||
Fixing this in bug 1652685.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•