Bug 208261 – Poly proto should work with property delete transitions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Michaud]

Poly proto should work with property delete transitions

[Justin Michaud]

Created attachment 391777 [details]
Patch

[Justin Michaud]

rdar://59302840

[Saam Barati]

Comment on attachment 391777 [details]
Patch

Can you see if we need this for get too? My hunch is yes

[Saam Barati]

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.

[Mark Lam]

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.

[Saam Barati]

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?

[Saam Barati]

Comment on attachment 391777 [details]
Patch

clearing r? for now, Justin has a forthcoming patch

[Justin Michaud]

Created attachment 391905 [details]
Patch

[Saam Barati]

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>

[Justin Michaud]

Created attachment 391943 [details]
Patch

[Mark Lam]

<rdar://problem/59302840>

[WebKit Commit Bot]

Comment on attachment 391943 [details]
Patch

Clearing flags on attachment: 391943

Committed r257605: <https://trac.webkit.org/changeset/257605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.