Bug 169704 - WebAssembly: When we GC to try to get a fast memory, we should call collectAllGarbage(), not collectSync()
Summary: WebAssembly: When we GC to try to get a fast memory, we should call collectAl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-15 15:36 PDT by Saam Barati
Modified: 2017-03-15 17:36 PDT (History)
11 users (show)

See Also:


Attachments
patch (1.85 KB, patch)
2017-03-15 15:41 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (1.96 KB, patch)
2017-03-15 15:42 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-03-15 15:36:37 PDT
Otherwise we won't actually end up getting the memory because we don't do any sweeping.
Comment 1 Saam Barati 2017-03-15 15:41:17 PDT
Created attachment 304563 [details]
patch
Comment 2 Saam Barati 2017-03-15 15:42:32 PDT
Created attachment 304564 [details]
patch
Comment 3 Mark Lam 2017-03-15 15:43:31 PDT
Comment on attachment 304564 [details]
patch

r=me
Comment 4 WebKit Commit Bot 2017-03-15 16:17:18 PDT
Comment on attachment 304564 [details]
patch

Clearing flags on attachment: 304564

Committed r214018: <http://trac.webkit.org/changeset/214018>
Comment 5 WebKit Commit Bot 2017-03-15 16:17:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Filip Pizlo 2017-03-15 16:35:52 PDT
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.
Comment 7 Saam Barati 2017-03-15 16:43:42 PDT
(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.
Comment 8 Filip Pizlo 2017-03-15 16:55:18 PDT
(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.
Comment 9 Saam Barati 2017-03-15 17:36:45 PDT
(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