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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file)

Attached patch patchSplinter 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 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+
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.

Attachment

General

Created:
Updated:
Size: