Bug 169704 – WebAssembly: When we GC to try to get a fast memory, we should call collectAllGarbage(), not collectSync()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

Otherwise we won't actually end up getting the memory because we don't do any sweeping.

[Saam Barati]

Created attachment 304563 [details]
patch

[Saam Barati]

Created attachment 304564 [details]
patch

[Mark Lam]

Comment on attachment 304564 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 304564 [details]
patch

Clearing flags on attachment: 304564

Committed r214018: <http://trac.webkit.org/changeset/214018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 304564 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmMemory.cpp:123
> -        vm.heap.collectSync();
> +        vm.heap.collectAllGarbage();
>          return dequeFastMemory();

This is sad - isn't this whole thing part of some promise anyway?  Seems like waiting for the GC to finish should be part of the promise.

[Saam Barati]

(In reply to comment #6)
> Comment on attachment 304564 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304564&action=review
> 
> > Source/JavaScriptCore/wasm/WasmMemory.cpp:123
> > -        vm.heap.collectSync();
> > +        vm.heap.collectAllGarbage();
> >          return dequeFastMemory();
> 
> This is sad - isn't this whole thing part of some promise anyway?  Seems
> like waiting for the GC to finish should be part of the promise.

I see what you're getting at here, but I think it's somewhat tricky to do. We would need a two-level strategy. Call collectSync, then set up a promise to be called when things get swept, then set up another promise when we're done compiling. I'm not sure exactly how to do that. It might be worth doing as part of Keith's work on async compilation, but we also don't expect this code to run very often.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 304564 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=304564&action=review
> > 
> > > Source/JavaScriptCore/wasm/WasmMemory.cpp:123
> > > -        vm.heap.collectSync();
> > > +        vm.heap.collectAllGarbage();
> > >          return dequeFastMemory();
> > 
> > This is sad - isn't this whole thing part of some promise anyway?  Seems
> > like waiting for the GC to finish should be part of the promise.
> 
> I see what you're getting at here, but I think it's somewhat tricky to do.
> We would need a two-level strategy. Call collectSync, then set up a promise
> to be called when things get swept, then set up another promise when we're
> done compiling. I'm not sure exactly how to do that. It might be worth doing
> as part of Keith's work on async compilation, but we also don't expect this
> code to run very often.

Right, we would need the incremental sweeper to run more aggressively and we would want it to notify us when done. 

Worth filing a bug.

[Saam Barati]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Comment on attachment 304564 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=304564&action=review
> > > 
> > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:123
> > > > -        vm.heap.collectSync();
> > > > +        vm.heap.collectAllGarbage();
> > > >          return dequeFastMemory();
> > > 
> > > This is sad - isn't this whole thing part of some promise anyway?  Seems
> > > like waiting for the GC to finish should be part of the promise.
> > 
> > I see what you're getting at here, but I think it's somewhat tricky to do.
> > We would need a two-level strategy. Call collectSync, then set up a promise
> > to be called when things get swept, then set up another promise when we're
> > done compiling. I'm not sure exactly how to do that. It might be worth doing
> > as part of Keith's work on async compilation, but we also don't expect this
> > code to run very often.
> 
> Right, we would need the incremental sweeper to run more aggressively and we
> would want it to notify us when done. 
> 
> Worth filing a bug.

Filed:
https://bugs.webkit.org/show_bug.cgi?id=169728