1336199 - Allow storing alternate data (ex: JS Bytecode) in the DOM cache

Status: NEW

(Core :: Storage: Cache API, defect, P3)

Description

[Andrew Sutherland [:asuth] (he/him)]

Bug 1231565 added support for the HTTP cache to store "alternative data type" streams in support of caching parsed JS representations (serialized in their XDR form) for bug 900784.

DOM cache should plan to support this so sites switching to ServiceWorkers don't experience performance regressions in JS loading versus a fully-HTTP cached site.

The implementation is likely to also be important to WASM in the future since it should allow the compiled form to be opportunistically or explicitly cached along-side the canonical body response.  (Currently, our IndexedDB implementation supports storage of compiled WASM modules via structured clone.  But this might not end up the standard way to do so... in https://github.com/WebAssembly/design/issues/972#issuecomment-276877682 the Google Chrome team has indicated they've disabled IndexedDB persistence of WASM modules.)  WASM is likely to favor directly receiving a file descriptor that could potentially be mmapp'ed rather than a stream, so that should ideally be factored into any implementation changes.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I had some thoughts on this in bug 900784 comment 66.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

HoPang is working on this, assigning to him, thanks!

Comment 3

[Nicolas B. Pierron [:nbp]]

If you need help for the API, Valentin Gosu did the implementation as part of the Necko, and I used it in the ScriptLoader.
If you want to test using the ScriptLoader, you can ask me, or have a look at the test case:
  http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/dom/base/test/test_script_loader_js_cache.html

Comment 4

[Ben Hsu [:HoPang]]

Thanks Nicolas,

The testcase indeed helped me a lot in bug 1350359 ;)

Comment 5

[Ben Hsu [:HoPang]]

Attachment: P1: Remove useless code (WIP)

Comment 6

[Ben Hsu [:HoPang]]

Attachment: P2: Save alternative data in local files (WIP)

Comment 7

[Ben Hsu [:HoPang]]

Attachment: P3: Save the filenames for alternative data to the datebase (WIP)

Comment 8

[Ben Hsu [:HoPang]]

Attachment: P4: IPDL (WIP)

Comment 9

[Ben Hsu [:HoPang]]

Hi Eden,

Though the patches cannot be applied directly, I believe they still can be used as references and reminder for the discussion we had a couple days ago. In additional to the patches, we still need to plumb the IPDL and modify the implementation of match() to retrieve the saved alternative data from the DOM cache. Besides, we still need a testcase for that, which could be similar to the one in bug 1350359. We just need to save the response carrying the alternative data to DOM cache and retrieve it.

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1336199 - P1: mochitest for loading JSBytcode form DOM cache. r?bkelly

Reusing the test case in test_script_loader_intercepted_js_cache.html.
In this test, file_js_dom_cache.html will be loaded three times.
For the first time, get the data by fetching from the network, and script loader will save the alternative data into http cache.
For the second time, get the data from http cache by calling fetch again, and ServiceWorkerScript will save the alternative data into the dom cache.
For the third time, get the response from dom cache directly. And the same result with the second time load is expected.

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Bug 1336199 - P2: Keeping alternative body and nsICacheInfoChannel in the InternalResponse. r?bkelly

Since alternativeBody and nsICacheInfoChannel will be accessed multiple times when synthesizing the response and saving the data into the dom cache, we need to keep them in InternalResponse.
This patch also supports clone the alternative data and cacheInfoChannel when calling InternalResponse::Clone.

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Bug 1336199 - P3: Saving alternative body and nsICacheInfoChannel into the dom cache in cache::PutAll. r?bkelly

This patch supports to save the alternative body and nsICacheInfoChannel information while putting request/response pair into the dom cache.
In fact, this patch doesn't save the whole nsICacheInfoChannel into the dom cache, it only saves the needed raw data for loading alternative body by script loader.

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: ug 1336199 - P4: Reading alternative body and nsICacheInfoChannel information from dom cache while cache::Match. r?bkelly

Supports to load saved alternative data and nsICacheInfoChannel information from the dom cache while executing match operations.
CachedCacheInfoChannel is a limited nsICacheInfoChannel implementation for script loader reading the alternative data, since it is hard to restore the real nsICacheInfoChannel without open a real channel.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry I didn't get to this review today.  I will do it tomorrow morning.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #14)
> I'm sorry I didn't get to this review today.  I will do it tomorrow morning.

I didn't do it the next morning!  I think I can say I won't have time for these reviews.  The patches should probably be rebased before review at this point as well.

Comment 16

[Luke Wagner [:luke]]

General question: if a Response is clone()d and one Response gets stored in the Cache and the other Response is fed to the JS script loader (or, with bug 1487113, WebAssembly.compileStreaming()) and thereby gains alt-data in the HTTP Cache, would the Cache.put() be able to "take" the alt-data from the HTTP Cache so that it doesn't have to be regenerated a second time?

Otherwise, to avoid this recompilation, iiuc, the developer would:
 cache.add(url).then(() => fetchEvent.respondWith(cache.match(url)))
which seems a bit sub-optimal.

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Talking with Eden, he's not actively working on this but still in his plan.

Comment 18

[Eden Chuang[:edenchuang]]

Not working on this. Remove the assignee.

Comment 19

[colormatch]

Example of how much this bug affects apps' start time.
from bug 1593579

Booting my app :
(Hiding the splash and showing GV. All content is delivered by serviceworker):

onFirstComposite
sequence after app-start:

• splash screen duration <1s
• black screen 0.7~3s
• base UI gets rendered (main page ~15k)
• delay ~0.4s
• toolbox UI get rendered (additional JS file ~200k)

onFirstContentfulPaint
sequence after app-start:

• splash screen duration 25s (in most cases 23s)
• base UI with toolbox UI is ready and visible

Note: that when onFirstContentfulPaint gets triggered, all the app content is ready/compiled - the toolbox UI from the additional JS file has been run and the toolbox UI rendered.

That means that the cost of not having the JS Bytecode cached/ready, means 3 times longer "booting time" for the app.
(tested on 2yr old, mid-range device)

Comment 21

[colormatch]

*correction

onFirstContentfulPaint
sequence after app-start:
splash screen duration is about 3s

Comment 24

[Jens Stutte [:jstutte]]

Is this still wanted and/or are those patches still useful ?

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

This is definitely still wanted. It might make sense to revisit the work in these patches after bug 1899507 and bug 1689149 are implemented, potentially keeping this use-case in mind for those use-cases. In particular, we potentially might not need to involve the necko channel machinery here if we're largely able to operate in the spec Response space and in terms of RemoteLazyInputStream and nsIInputStreams at the implementation level (with the ability to provide a file descriptor directly to JS for memory mapping in unencrypted cases).

Comment 26

[Eden Chuang[:edenchuang]]

I will back to rebase these patches first, then investiage what we should with them.

Comment 27

[Eden Chuang[:edenchuang]]

ni me for comment 26