[DFG][FTL] WeakMap#set should have DFG node

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

And we should make it participate in CSE.

[Yusuke Suzuki]

Created attachment 329759 [details]
Patch

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 329759 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:21
> +        WeakMap operations do not cause GC.

Maybe it's worth documenting this in WeakMapImpl that the compiler depends on this now.

> Source/JavaScriptCore/dfg/DFGClobberize.h:1666
> +    case WeakSetAdd: {

I wonder if it's worth commenting on the subtlety here, since intervening operations could have caused a GC. Since we're keeping the key alive though, it's guaranteed that CSE should return the actual state of the map, since if the key is alive, and we prove it's there via CSE, that must be the state of the map. I guess there are times we should have code like this:

function foo(s, k) {
    let a = s.has(k);
    new Array; // causes a GC, and clears k
    let b = s.has(k); // this turns into nothing, and we don't conservatively keep k alive
}

both b and a may be true, even though the GC may have cleared k from the map. But, I think this is fine semantically in JS. The only thing that would not be semantically OK would be to observe some sort of time travel in result operations, but I don't think that's possible, because it wouldn't be possible via liveness of the key to do such a thing.

[Yusuke Suzuki]

Comment on attachment 329759 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:21
>> +        WeakMap operations do not cause GC.
> 
> Maybe it's worth documenting this in WeakMapImpl that the compiler depends on this now.

OK, done. We also ensured this by using DisallowGC in WeakMapImpl.

>> Source/JavaScriptCore/dfg/DFGClobberize.h:1666
>> +    case WeakSetAdd: {
> 
> I wonder if it's worth commenting on the subtlety here, since intervening operations could have caused a GC. Since we're keeping the key alive though, it's guaranteed that CSE should return the actual state of the map, since if the key is alive, and we prove it's there via CSE, that must be the state of the map. I guess there are times we should have code like this:
> 
> function foo(s, k) {
>     let a = s.has(k);
>     new Array; // causes a GC, and clears k
>     let b = s.has(k); // this turns into nothing, and we don't conservatively keep k alive
> }
> 
> both b and a may be true, even though the GC may have cleared k from the map. But, I think this is fine semantically in JS. The only thing that would not be semantically OK would be to observe some sort of time travel in result operations, but I don't think that's possible, because it wouldn't be possible via liveness of the key to do such a thing.

Yeah. As long as the latter `s.has(k)` returns the same value to the former `s.has(k)`, it does not break semantics.
If `a` = true and `b` = false, it is semantically incorrect since `k` should be live when querying in `s.has(k)`. But it never happens with this CSE.

[Yusuke Suzuki]

Committed r227723: <https://trac.webkit.org/changeset/227723>

[Radar WebKit Bug Importer]

<rdar://problem/36978679>