Bug 167017 – Initialize the ArraySpecies watchpoint as Clear and transition to IsWatched once slice is called for the first time

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

This seems to fix the JSBench regression.

[Radar WebKit Bug Importer]

<rdar://problem/30019309>

[Filip Pizlo]

Fascinating!

What does it mean for the watchpoint to be lazy?

[Keith Miller]

(In reply to comment #2)
> Fascinating!
> 
> What does it mean for the watchpoint to be lazy?

It means that we don't initialize the watchpoint until the function using it is called.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > Fascinating!
> > 
> > What does it mean for the watchpoint to be lazy?
> 
> It means that we don't initialize the watchpoint until the function using it
> is called.

By using, you mean watching or...?

Is this an alternate way of saying whether the watchpoint is initialized watched or if the watchpoint is blind initially?

[Saam Barati]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Fascinating!
> > > 
> > > What does it mean for the watchpoint to be lazy?
> > 
> > It means that we don't initialize the watchpoint until the function using it
> > is called.
> 
> By using, you mean watching or...?
> 
> Is this an alternate way of saying whether the watchpoint is initialized
> watched or if the watchpoint is blind initially?

The watchpoint is blind initially.
So we start it off as ClearWatchpoint.

From ClearWatchpoint, once slice is called for the first time, we can either do:
ClearWatchpoint => Invalidated
ClearWatchpoint => IsWatched

depending on the state of the world. We do this transition when slice is called for the fist time and reaches a certain execution point (past some number of exception checks regarding toNumber() on parameters).

[Saam Barati]

Created attachment 298786 [details]
patch

[WebKit Commit Bot]

Attachment 298786 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 298786 [details]
patch

Super cool.  Looks good to me.

[Keith Miller]

Comment on attachment 298786 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298786&action=review

r=me too.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:-1362
> -    ASSERT(constructorSlot.slotBase() == this);
> -    ASSERT(constructorSlot.isCacheableValue());

I'm surprised I put these assertions in, they are very clearly wrong... since someone could have changed these before we set the watchpoint.

[Saam Barati]

(In reply to comment #9)
> Comment on attachment 298786 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298786&action=review
> 
> r=me too.
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:-1362
> > -    ASSERT(constructorSlot.slotBase() == this);
> > -    ASSERT(constructorSlot.isCacheableValue());
> 
> I'm surprised I put these assertions in, they are very clearly wrong...
> since someone could have changed these before we set the watchpoint.
You didn't. I made them assertions because I switched this function to run from global object construction. But since that's no longer the case, I moved back to the checks.

[Geoffrey Garen]

Comment on attachment 298786 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298786&action=review

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1411
> +    // We only watch this from the DFG, and the DFG makes sure to only start watching if the watchpoint is in the IsWatched state.
> +    RELEASE_ASSERT(!globalObject->arraySpeciesWatchpoint().isBeingWatched()); 

Code seems to say is *not* in the IsWatched state.

[Saam Barati]

(In reply to comment #11)
> Comment on attachment 298786 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298786&action=review
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1411
> > +    // We only watch this from the DFG, and the DFG makes sure to only start watching if the watchpoint is in the IsWatched state.
> > +    RELEASE_ASSERT(!globalObject->arraySpeciesWatchpoint().isBeingWatched()); 
> 
> Code seems to say is *not* in the IsWatched state.

This function name is somewhat confusing, but it's not talking about IsWatched state, it's talking about if anybody is on the Watchpoint's list.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/210745