Summary: | Prototype structure transition should be a deferred transition | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | Safari Technology Preview | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Saam Barati
2017-10-01 23:54:43 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. (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. Created attachment 323358 [details]
patch
Comment on attachment 323358 [details]
patch
r=me.
Comment on attachment 323358 [details] patch Clearing flags on attachment: 323358 Committed r223161: <http://trac.webkit.org/changeset/223161> All reviewed patches have been landed. Closing bug. |