Closed
Bug 1116017
Opened 9 years ago
Closed 9 years ago
Don't scan all type sets in compartments on type mutations
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file)
74.69 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
We've had this behavior for as long as TI has been around that when an object's type mutates (by changing its __proto__ or by swapping it with another object) then unless we already know type sets containing the object are all unknownObject(), we walk through the entire compartment and mark such sets as unknownObject(). This has been fine for the above cases where an object's type mutates, since they're pretty pathological, but for bug 1106828 we will have cases where type mutations can occur during execution of 'normal' code. It would be nice to not have to crawl the compartment in these cases, which means relaxing the meaning of a type set sufficiently so that crawling doesn't happen during prototype mutation. The attached patch makes this change --- type sets are not required to contain types for every value in the lvalues they represent, but can be missing an object type so long as they contain some object with unknown properties. This relaxes one of the fundamental TI invariants, but there isn't a lot of fallout. When mutating a type the old and new type both have unknown properties, which makes it pretty much impossible to optimize them within Ion. The main changes are to fix the places where we would still optimize such objects based on either their class or their proto, parts of a TypeObjectKey which can no longer be considered immutable within IonBuilder.
Attachment #8541951 -
Flags: review?(jdemooij)
Comment 1•9 years ago
|
||
Comment on attachment 8541951 [details] [diff] [review] patch Review of attachment 8541951 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Glad we can remove the compartment walk and this looks pretty straight-forward actually. ::: js/src/jit/IonBuilder.cpp @@ +5702,5 @@ > if (!curType) > continue; > > + if (curType->hasStableClassAndProto(constraints())) > + return false; Shouldn't this be: if (!..) (missing '!') ::: js/src/jit/MIR.cpp @@ +327,5 @@ > resumePoint_->resetInstruction(); > block()->discardPreAllocatedResumePoint(resumePoint_); > resumePoint_ = nullptr; > } > Can you change MakeSingletonTypeSet in this file to call hasStableClassAndProto? Currently it calls hasFlags directly so it should be exactly the same, it just seems clearer to use hasStableClassAndProto now. ::: js/src/jit/MIR.h @@ +2423,5 @@ > // to do that from New() or the constructor, since those can be called on > // background threads. So make callers explicitly call it if they want us > // to check whether the operand might do this. If this method is never > // called, we'll assume our operand can emulate undefined. > + void cacheOperandMightEmulateUndefined(types::CompilerConstraintList *constraints); Having a `constraints` argument here is really nice actually, to keep people from calling this method or getKnownClass() etc off-thread :)
Attachment #8541951 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 2•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cec093aeadc
Comment 3•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cec093aeadc
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•