Bug 224092

Summary: Introduce ScriptBuffer class to wrap SharedBuffer containing a script
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, darin, ews-watchlist, ggaren, gyuyoung.kim, hi, joepeck, mkwst, ryuan.choi, sergio, simon.fraser, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=234215
Bug Depends on: 224015    
Bug Blocks: 224088    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description Chris Dumez 2021-04-01 19:42:53 PDT
Introduce ScriptBuffer class to SharedBuffer containing a script. We started using SharedBuffer to represent worker scripts instead of String, so that they can hold file mapped data and be shared across processes.
This patch introduces a new ScriptBuffer to wrap those SharedBuffers. The type makes it clearer what type of data we're dealing with. The helper functions used to convert between String and SharedBuffer can now simply be member functions on ScriptBuffer.
This also simplifies IPC code.
Comment 1 Chris Dumez 2021-04-01 20:16:29 PDT
Created attachment 424983 [details]
Patch
Comment 2 Chris Dumez 2021-04-01 21:08:09 PDT
Created attachment 424984 [details]
Patch
Comment 3 Geoffrey Garen 2021-04-02 13:34:55 PDT
Can we use ScriptBuffer for regular scripts in the HTTP cache too?
Comment 4 Chris Dumez 2021-04-02 13:37:08 PDT
(In reply to Geoffrey Garen from comment #3)
> Can we use ScriptBuffer for regular scripts in the HTTP cache too?

The HTTP cache serves many different types of files, not just scripts. I don't think it would be practical to use it there. The HTTP cache uses ShareableResource::Handle directly.
Comment 5 Yusuke Suzuki 2021-04-02 13:37:44 PDT
Comment on attachment 424984 [details]
Patch

r=me
Comment 6 EWS 2021-04-02 13:38:24 PDT
Tools/Scripts/svn-apply failed to apply attachment 424984 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 7 Geoffrey Garen 2021-04-02 13:40:08 PDT
r=me

It's nice to get this improvement for workers, but it really makes me wonder what we do for other scripts, and if we can share some code.
Comment 8 Chris Dumez 2021-04-02 13:41:54 PDT
Created attachment 425050 [details]
Patch
Comment 9 Chris Dumez 2021-04-02 13:51:22 PDT
(In reply to Geoffrey Garen from comment #7)
> r=me
> 
> It's nice to get this improvement for workers, but it really makes me wonder
> what we do for other scripts, and if we can share some code.

Main thread scripts are not really treated different from other resources. They are stored in the memory cache as a SharedBuffer inside the CachedResource. Once the network process has saved the resource to disk, it sends a URL + ShareableResource::Handle to the WebProcess. The WebProcess looks up the CachedResource using the URL in the memory cache, then tries and replace the CachedResource's SharedBuffer with the file-mapped version.

This logic does not currently work with dedicated or service workers because they do not use CachedScript or the memory cache. The memory cache is currently main-thread only.

Also, service workers are interestingly different (than usual network resources or even dedicated worker scripts) because they are persisted on disk with the service worker registration, independently from the HTTP disk cache. Therefore, it is highly preferable to use a version of the service worker script file-mapped from the service worker storage on disk, rather than the HTTP disk cache.

For dedicated workers, we should in theory be able to leverage the existing NetworkProcessConnection::DidCacheResource IPC to replace the in-memory version of the script with the file-mapped version from the HTTP disk cache. I have not worked on this part yet and I have been focusing on Service Workers since this is what's covered by PLUM3.
Comment 10 EWS 2021-04-02 14:53:36 PDT
Committed r275443: <https://commits.webkit.org/r275443>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425050 [details].
Comment 11 Radar WebKit Bug Importer 2021-04-02 14:54:17 PDT
<rdar://problem/76166419>
Comment 12 Geoffrey Garen 2021-04-02 15:57:01 PDT
> They are stored in the memory cache as a SharedBuffer inside the
> CachedResource. Once the network process has saved the resource to disk, it
> sends a URL + ShareableResource::Handle to the WebProcess. The WebProcess
> looks up the CachedResource using the URL in the memory cache, then tries
> and replace the CachedResource's SharedBuffer with the file-mapped version.

I see: CachedScript has a dirty string m_script; but that's the decoded data that we throw away periodically. The encoded data is already SharedBuffer.