Bug 177734 - Prototype structure transition should be a deferred transition
Summary: Prototype structure transition should be a deferred transition
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-01 23:54 PDT by Saam Barati
Modified: 2017-10-10 17:55 PDT (History)
13 users (show)

See Also:


Attachments
patch (5.61 KB, patch)
2017-10-10 16:42 PDT, Saam Barati
no flags 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-10-01 23:54:43 PDT
I think OPC set depends on the structure being changed before the watchpoint fires. If true, let's make it use the deferred transition mechanism.
Comment 1 Saam Barati 2017-10-10 15:32:26 PDT
So I was right that this was wrong, however, we catch ourselves from bugs because this check:

```bool PropertyCondition::isWatchableWhenValid(
    Structure* structure, WatchabilityEffort effort) const
{
    if (structure->transitionWatchpointSetHasBeenInvalidated()) {
        return false;
    }
    ...
}````

We will always return false here because the Structure in question just transitioned by changing its prototype, therefore, it will always have an invalidated transition watchpoint set. The consequence here is if we always set our prototype to be the same object, we'll end up invalidating these watchpoints repeatedly.
Comment 2 Saam Barati 2017-10-10 16:23:52 PDT
(In reply to Saam Barati from comment #1)
> So I was right that this was wrong, however, we catch ourselves from bugs
> because this check:
> 
> ```bool PropertyCondition::isWatchableWhenValid(
>     Structure* structure, WatchabilityEffort effort) const
> {
>     if (structure->transitionWatchpointSetHasBeenInvalidated()) {
>         return false;
>     }
>     ...
> }````
> 
> We will always return false here because the Structure in question just
> transitioned by changing its prototype, therefore, it will always have an
> invalidated transition watchpoint set. The consequence here is if we always
> set our prototype to be the same object, we'll end up invalidating these
> watchpoints repeatedly.

We actually skip transitioning if we set the prototype to the same value. This makes sense. Anyways, I'm going to make this change for sanity reasons. It seems like we've been relying on the wrong code path all along.
Comment 3 Saam Barati 2017-10-10 16:42:07 PDT
Created attachment 323358 [details]
patch
Comment 4 Keith Miller 2017-10-10 16:47:00 PDT
Comment on attachment 323358 [details]
patch

r=me.
Comment 5 WebKit Commit Bot 2017-10-10 17:54:03 PDT
Comment on attachment 323358 [details]
patch

Clearing flags on attachment: 323358

Committed r223161: <http://trac.webkit.org/changeset/223161>
Comment 6 WebKit Commit Bot 2017-10-10 17:54:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-10-10 17:55:39 PDT
<rdar://problem/34924463>