Poly proto should work with property delete transitions
Created attachment 391777 [details] Patch
rdar://59302840
Comment on attachment 391777 [details] Patch Can you see if we need this for get too? My hunch is yes
Comment on attachment 391777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391777&action=review > JSTests/stress/delete-property-poly-proto.js:1 > +//@ runDefault("--forcePolyProto=1 --jitPolicyScale=0") I think this is wrong, if you see how we do it elsewhere, it's: runDefault("--forcePolyProto=1", "--jitPolicyScale=0") Also, I think the safer test here is to run without LLInt, so we're guaranteed to be inside baseline.
Comment on attachment 391777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391777&action=review >> JSTests/stress/delete-property-poly-proto.js:1 >> +//@ runDefault("--forcePolyProto=1 --jitPolicyScale=0") > > I think this is wrong, if you see how we do it elsewhere, it's: > runDefault("--forcePolyProto=1", "--jitPolicyScale=0") > > Also, I think the safer test here is to run without LLInt, so we're guaranteed to be inside baseline. The better way to do this is: //@ requireOptions("--forcePolyProto=1", "--jitPolicyScale=0") That will run it on all the test configurations instead of just the default one. Unless there's a special reason why you want to limit this to only run the default configuration, just use requireOptions. That way, we'll get maximal test coverage.
Comment on attachment 391777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391777&action=review > Source/JavaScriptCore/ChangeLog:7 > + can you explain what's happening?
Comment on attachment 391777 [details] Patch clearing r? for now, Justin has a forthcoming patch
Created attachment 391905 [details] Patch
Comment on attachment 391905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391905&action=review r=me > Source/JavaScriptCore/ChangeLog:9 > + and poly photo cause us to cache a setter call along a prototype chain that photo => proto > Source/JavaScriptCore/ChangeLog:10 > + is no longer the correct setter to call. This is exposed as a result of 206430 nit: I'd link to a revision instead of "206430", since "206430" is slightly ambiguous without a link > Source/JavaScriptCore/ChangeLog:24 > + were before it was called. We see that A's setter still exists, so we cache it > + without ever checking that it is still a setter. This sentence doesn't make sense. I think you also need to say what your fix actually is. All you're doing is describing a problem. Can you also link to bugs for fixing the semantic issues that remain? > JSTests/stress/delete-property-poly-proto.js:1 > +//@ requireOptions("--forcePolyProto=1", "--useLLInt=0") nit: use 4 space indent > JSTests/stress/poly-proto-setter-adds-setter-in-middle.js:32 > +//if (!correct) > +// throw new Error() I think you shouldn't have commented out code here, instead, you should do: FIXME: Assert that correct is true <bug link here>
Created attachment 391943 [details] Patch
<rdar://problem/59302840>
Comment on attachment 391943 [details] Patch Clearing flags on attachment: 391943 Committed r257605: <https://trac.webkit.org/changeset/257605>
All reviewed patches have been landed. Closing bug.