Bug 224092 - Introduce ScriptBuffer class to wrap SharedBuffer containing a script
Summary: Introduce ScriptBuffer class to wrap SharedBuffer containing a script
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 224015
Blocks: 224088
  Show dependency treegraph
 
Reported: 2021-04-01 19:42 PDT by Chris Dumez
Modified: 2021-04-02 15:57 PDT (History)
13 users (show)

See Also:


Attachments
Patch (74.87 KB, patch)
2021-04-01 20:16 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (75.39 KB, patch)
2021-04-01 21:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (75.24 KB, patch)
2021-04-02 13:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.