Summary: | Poly proto should work with property delete transitions | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Michaud <justin_michaud> | ||||||||
Component: | New Bugs | Assignee: | Justin Michaud <justin_michaud> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 208337 | ||||||||||
Attachments: |
|
Description
Justin Michaud
2020-02-26 14:04:36 PST
Created attachment 391777 [details]
Patch
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
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. |