Bug 167017 - Initialize the ArraySpecies watchpoint as Clear and transition to IsWatched once slice is called for the first time
Summary: Initialize the ArraySpecies watchpoint as Clear and transition to IsWatched o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-13 13:46 PST by Saam Barati
Modified: 2017-01-13 15:43 PST (History)
13 users (show)

See Also:


Attachments
patch (11.31 KB, patch)
2017-01-13 14:19 PST, Saam Barati
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-01-13 13:46:28 PST
This seems to fix the JSBench regression.
Comment 1 Radar WebKit Bug Importer 2017-01-13 13:48:04 PST
<rdar://problem/30019309>
Comment 2 Filip Pizlo 2017-01-13 13:50:49 PST
Fascinating!

What does it mean for the watchpoint to be lazy?
Comment 3 Keith Miller 2017-01-13 13:53:03 PST
(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.
Comment 4 Filip Pizlo 2017-01-13 13:54:08 PST
(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?
Comment 5 Saam Barati 2017-01-13 13:57:14 PST
(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).
Comment 6 Saam Barati 2017-01-13 14:19:35 PST
Created attachment 298786 [details]
patch
Comment 7 WebKit Commit Bot 2017-01-13 14:20:43 PST
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.
Comment 8 Filip Pizlo 2017-01-13 14:23:04 PST
Comment on attachment 298786 [details]
patch

Super cool.  Looks good to me.
Comment 9 Keith Miller 2017-01-13 14:34:19 PST
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.
Comment 10 Saam Barati 2017-01-13 14:49:36 PST
(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.
Comment 11 Geoffrey Garen 2017-01-13 15:38:27 PST
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.
Comment 12 Saam Barati 2017-01-13 15:41:04 PST
(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.
Comment 13 Saam Barati 2017-01-13 15:43:39 PST
landed in:
https://trac.webkit.org/changeset/210745