We are too conservative about the effects of PushWithScope

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

In https://bugs.webkit.org/show_bug.cgi?id=175470, Saam says that it is safe to remove the annotation that PushWithScope clobbers the world.
This bug is just for tracking the removal of this annotation.

[Robin Morisset]

Created attachment 318133 [details]
Patch

[Saam Barati]

Comment on attachment 318133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318133&action=review

> Source/JavaScriptCore/ChangeLog:7
> +

It's worth a sentence describing the reasoning here.

Given the semantics of PushWithScope, I think it's also worth changing Clobberize and DoesGC (based on making Clobberize not report it clobbers things).

Can you also add tests that would stress a few things:
- we CSE around a PushWithScope
- cases where PushWithScope throws an exception in the DFG because the "o" is null/undefined
- cases where PushWithScope allocates (e.g, the "o" is a number/string/Symbol)

[Robin Morisset]

Created attachment 318210 [details]
Patch

[Saam Barati]

Comment on attachment 318210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318210&action=review

> Source/JavaScriptCore/dfg/DFGClobberize.h:475
> +        read(ScopeProperties);

This is not the case, this node does not read ScopeProperties. This heap kind is for GetClosureVar, PutClosureVar.

Also, please add tests that stress the CSE this opens up.

[Saam Barati]

Comment on attachment 318210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318210&action=review

> Source/JavaScriptCore/dfg/DFGClobberize.h:476
> +        read(HeapObjectCount);

This reads more things. For example, it needs to read at least enough things to do a type check, to JSCell_typeInfo, JSCell_structureID, etc
Maybe it's good to just say read(World) for now

[Robin Morisset]

Created attachment 318216 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 318216 [details]
Patch

Clearing flags on attachment: 318216

Committed r220783: <http://trac.webkit.org/changeset/220783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.