Bug 189043 - [WebAssembly] Use StreamingParser in existing Wasm::BBQPlan
Summary: [WebAssembly] Use StreamingParser in existing Wasm::BBQPlan
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-27 23:51 PDT by Yusuke Suzuki
Modified: 2019-09-10 02:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (42.36 KB, patch)
2018-08-28 00:00 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (42.41 KB, patch)
2018-08-28 03:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (42.36 KB, patch)
2018-08-28 07:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (42.72 KB, patch)
2019-08-18 03:04 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (45.55 KB, patch)
2019-09-02 14:01 PDT, Yusuke Suzuki
keith_miller: 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 2018-08-27 23:51:13 PDT
[WebAssembly] Use StreamingParser in existing Wasm::BBQPlan
Comment 1 Yusuke Suzuki 2018-08-28 00:00:54 PDT
Created attachment 348269 [details]
Patch
Comment 2 Yusuke Suzuki 2018-08-28 03:54:33 PDT
Created attachment 348287 [details]
Patch
Comment 3 Yusuke Suzuki 2018-08-28 07:49:26 PDT
Created attachment 348292 [details]
Patch
Comment 4 Yusuke Suzuki 2019-08-02 20:22:52 PDT
I also would like to revive this patch to prepare for introducing compileStreaming / instantiateStreaming.
Comment 5 Yusuke Suzuki 2019-08-18 03:04:50 PDT
Created attachment 376631 [details]
Patch

WIP
Comment 6 Yusuke Suzuki 2019-09-02 14:01:05 PDT
Created attachment 377857 [details]
Patch
Comment 7 Keith Miller 2019-09-03 11:27:38 PDT
Comment on attachment 377857 [details]
Patch

The phrasing of the callback names is a little weird. didParseFunction makes it seem like it's either a query (i.e. has this function already been parsed) or that the caller already parsed the function and is informing the client it has been parsed. Is the expectation that the callbacks are invoked when the relevant bytes are available? If so, what do you think of changing the names to provide<Thing> e.g. provideFunction?
Comment 8 Yusuke Suzuki 2019-09-03 13:44:59 PDT
(In reply to Keith Miller from comment #7)
> Comment on attachment 377857 [details]
> Patch
> 
> The phrasing of the callback names is a little weird. didParseFunction makes
> it seem like it's either a query (i.e. has this function already been
> parsed) or that the caller already parsed the function and is informing the
> client it has been parsed. Is the expectation that the callbacks are invoked
> when the relevant bytes are available? If so, what do you think of changing
> the names to provide<Thing> e.g. provideFunction?

This naming convention is following the naming convention of this type of callback clients in WebCore.
For example, WebCore::ResourceHandleClient is a client of ResourceHandle, and its virtual functions are invoked when ResourceHandle gets some state. And the methods of ResourceHandleClient are like,

didSendData
didReceiveData
didFinishLoading

In WebCore, this type of clients are having methods like `didXXX` or `willXXX`. Here, I'm aligning the names to WebCore's naming convention for consistency.
Comment 9 Keith Miller 2019-09-03 13:57:01 PDT
(In reply to Yusuke Suzuki from comment #8)
> (In reply to Keith Miller from comment #7)
> > Comment on attachment 377857 [details]
> > Patch
> > 
> > The phrasing of the callback names is a little weird. didParseFunction makes
> > it seem like it's either a query (i.e. has this function already been
> > parsed) or that the caller already parsed the function and is informing the
> > client it has been parsed. Is the expectation that the callbacks are invoked
> > when the relevant bytes are available? If so, what do you think of changing
> > the names to provide<Thing> e.g. provideFunction?
> 
> This naming convention is following the naming convention of this type of
> callback clients in WebCore.
> For example, WebCore::ResourceHandleClient is a client of ResourceHandle,
> and its virtual functions are invoked when ResourceHandle gets some state.
> And the methods of ResourceHandleClient are like,
> 
> didSendData
> didReceiveData
> didFinishLoading
> 
> In WebCore, this type of clients are having methods like `didXXX` or
> `willXXX`. Here, I'm aligning the names to WebCore's naming convention for
> consistency.

If we want to stay consistent with WebCore's naming can we call it didReceiveFunctionData, didReceiveSectionData, etc? The current naming makes it sound like the parsing of the function has already happened when the function is called.
Comment 10 Yusuke Suzuki 2019-09-03 14:51:54 PDT
(In reply to Keith Miller from comment #9)
> (In reply to Yusuke Suzuki from comment #8)
> > (In reply to Keith Miller from comment #7)
> > > Comment on attachment 377857 [details]
> > > Patch
> > > 
> > > The phrasing of the callback names is a little weird. didParseFunction makes
> > > it seem like it's either a query (i.e. has this function already been
> > > parsed) or that the caller already parsed the function and is informing the
> > > client it has been parsed. Is the expectation that the callbacks are invoked
> > > when the relevant bytes are available? If so, what do you think of changing
> > > the names to provide<Thing> e.g. provideFunction?
> > 
> > This naming convention is following the naming convention of this type of
> > callback clients in WebCore.
> > For example, WebCore::ResourceHandleClient is a client of ResourceHandle,
> > and its virtual functions are invoked when ResourceHandle gets some state.
> > And the methods of ResourceHandleClient are like,
> > 
> > didSendData
> > didReceiveData
> > didFinishLoading
> > 
> > In WebCore, this type of clients are having methods like `didXXX` or
> > `willXXX`. Here, I'm aligning the names to WebCore's naming convention for
> > consistency.
> 
> If we want to stay consistent with WebCore's naming can we call it
> didReceiveFunctionData, didReceiveSectionData, etc? The current naming makes
> it sound like the parsing of the function has already happened when the
> function is called.

Sounds nice. I'll change the names to didReceiveFunctionData, didReceiveSectionData.
Comment 11 Keith Miller 2019-09-03 17:07:50 PDT
Comment on attachment 377857 [details]
Patch

r=me with the name change.
Comment 12 Yusuke Suzuki 2019-09-10 02:29:02 PDT
(In reply to Keith Miller from comment #11)
> Comment on attachment 377857 [details]
> Patch
> 
> r=me with the name change.

Thanks, I'll land with name change.
Comment 13 Yusuke Suzuki 2019-09-10 02:42:20 PDT
Committed r249708: <https://trac.webkit.org/changeset/249708>
Comment 14 Radar WebKit Bug Importer 2019-09-10 02:43:20 PDT
<rdar://problem/55216526>