Bug 180015 - [DFG][FTL] WeakMap#set should have DFG node
Summary: [DFG][FTL] WeakMap#set should have DFG node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 179929
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-26 06:56 PST by Yusuke Suzuki
Modified: 2018-01-29 01:26 PST (History)
6 users (show)

See Also:


Attachments
Patch (29.38 KB, patch)
2017-12-19 06:48 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-11-26 06:56:51 PST
And we should make it participate in CSE.
Comment 1 Yusuke Suzuki 2017-12-19 06:48:45 PST
Created attachment 329759 [details]
Patch
Comment 2 Yusuke Suzuki 2018-01-22 17:03:41 PST
Ping?
Comment 3 Saam Barati 2018-01-28 23:54:11 PST
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.
Comment 4 Yusuke Suzuki 2018-01-29 01:22:54 PST
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.
Comment 5 Yusuke Suzuki 2018-01-29 01:25:41 PST
Committed r227723: <https://trac.webkit.org/changeset/227723>
Comment 6 Radar WebKit Bug Importer 2018-01-29 01:26:27 PST
<rdar://problem/36978679>