New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 768705 link

Starred by 6 users
Status: Fixed
Owner:
horo@chromium.org
Closed: Apr 2018
Cc:
falken@chromium.org
kinuko@chromium.org
yangguo@chromium.org
addyo@chromium.org
slightlyoff@chromium.org
verwa...@chromium.org
█ kenjibaheux@chromium.org
█ vdje...@fb.com
rmcilroy@chromium.org
█ n...@fb.com
marja@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 787736


Sign in to add a comment

Service Workers should consider eagerly compiling and caching code when adding scripts to the cache.

Project Member Reported by yangguo@chromium.org, Sep 26 2017 
Currently, scripts in a Service Worker's cache is not compiled until it is run the first time. Even then, we only produce code cache for the top-level function (and IIFEs).

Maybe we should do more:
- Compile the whole script eagerly, including inner functions, and producing the code cache for that.
- Compile and produce code cache upon adding to the cache and not at the first run. Compiling and caching on first run would be on the critical path to loading.

Comment 1 by kinuko@chromium.org, Sep 26 2017

Cc: n...@fb.com
+n8s@fb

Comment 2 by marja@chromium.org, Sep 26 2017 

FYI, there was a discussion doc about this feature:

https://docs.google.com/document/d/1d8EeIY7nWpVT7yyofyqQuy57rQEf4Ft0s0virCSxUWI/edit#

(An earlier doc, Ahttps://docs.google.com/document/d/1oOvUos_1Vl2DUNWEYKkPeABvK0bnY5HQwfI972JKFuY/edit#heading=h.3m20v86ecbgn , proposed and explicit API for this purpose, but it didn't fly...)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/7f9de3dce97ca4ec252de3eb8b2e2de7da615789

commit 7f9de3dce97ca4ec252de3eb8b2e2de7da615789
Author: Yang Guo <yangguo@chromium.org>
Date: Tue Sep 26 09:53:53 2017

[snapshot] add kProduceExhaustiveCodeCache option.

- Add kProduceExhaustiveCodeCache to v8::ScriptCompiler::CompileOptions
  to request eager compilation to add as much as possible to the code
  cache for the script.
- Repurpose ParseInfo::kLazy flag.
- Remove ParseInfo::kDebug flag.
- Remove --serialize-toplevel as it has become obsolete.

R=marja@chromium.org

Bug:  chromium:768705 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Ife14f7a1d1c02e525f0b9dbfd2452013d67c7167
Reviewed-on: https://chromium-review.googlesource.com/684019
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48160}
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/include/v8.h
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/src/api.cc
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/src/background-parsing-task.cc
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/src/compiler.cc
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/src/flag-definitions.h
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/src/parsing/parse-info.h
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/src/parsing/parser.cc
[modify] https://crrev.com/7f9de3dce97ca4ec252de3eb8b2e2de7da615789/test/cctest/test-serialize.cc

Comment 4 by yangguo@chromium.org, Sep 26 2017 

For the record, I actually renamed the new option to kProduceFullCodeCache instead of kProduceExhaustiveCodeCache as the change description makes believe.

Comment 5 by n...@fb.com, Sep 26 2017

Labels: DevRel-Facebook

Comment 6 by vdje...@fb.com, Sep 27 2017

Cc: vdje...@fb.com

Comment 7 by yangguo@chromium.org, Sep 27 2017 

With the change in #3 landed, I consider the V8 part of this done. Rest of necessary changes should be in Blink. Please let me know though if there is anything missing on V8's end.

Comment 8 by kinuko@chromium.org, Sep 27 2017

Components: Blink>ServiceWorker
Status: Untriaged (was: Available)
Temporarily moving this back to Untriaged (and adding Blink>ServiceWorker) to put it in the triage queue.

Comment 9 by horo@chromium.org, Sep 29 2017

Components: Blink>Storage>CacheStorage
Owner: horo@chromium.org
Status: Assigned (was: Untriaged)
I will take this.

Comment 10 by horo@chromium.org, Oct 26 2017 

I created a proof of concept patch:
https://chromium-review.googlesource.com/c/chromium/src/+/737595
The layouttest "service-worker-v8-cache.html" tests that the code cache of v8-cache-script.js is generated in the install event handler.

yangguo@
I have one question about v8::Isolate.
In this CL, I used the v8::Isolate of ServiceWorker's ScriptState to call v8::ScriptCompiler::Compile().
https://chromium-review.googlesource.com/c/chromium/src/+/737595/6/third_party/WebKit/Source/modules/cachestorage/Cache.cpp#456
But is there any side effect on the Service Worker's environment?
Or do I need to create another v8::Isolate to generate the code cache?

Comment 11 by yangguo@chromium.org, Oct 30 2017 

Compiling a script creates objects on that Isolate's heap, which should be garbage collected later. It should not have any side effect observable from JavaScript. To be extra sure, you should use v8::ScriptCompiler::CompileUnbound, which does not require a v8::Context, and therefore should not affect any environment.

Comment 12 by horo@chromium.org, Oct 31 2017 

yangguo@
Thank you for your comment. I changed the CL to use v8::ScriptCompiler::CompileUnboundScript.

Comment 13 by horo@chromium.org, Oct 31 2017 

I briefly measured the performance impact of full code cache in Nexus 4.
FMP(First meaningful paint) time including SW startup of Twitter Lite was improved from 6.38 sec to 4.84 sec.
https://docs.google.com/drawings/d/1SkTvByL--lR--lfmxrXVrev7-5SOViquLQbCB6pHNSw/edit

Comment 15 by bke...@mozilla.com, Nov 9 2017 

Sorry, I realize this is an internal optimization issue, but I saw this in the doc:

"But to provide the Web developers with a method to intentionally generate code cache without installing a new Service Worker, it may be OK to generate the code cache when cache.put() is called in the page."

FWIW, I've had similar conversations with our WASM folks at Mozilla and I would prefer an explicit API instead of magic.  Something like `WebAssembly.Compile(cache, request)` seems like a nice way to explicitly say "compile this entry in the Cache".  Of course, I'm not sure if I convinced our WASM folks of this or not.

Anyway, just wanted to mention this since you seem to be thinking about something explicit at some point.  Thanks.

Comment 16 by horo@chromium.org, Nov 9 2017 

That is interesting.
Thank you for the information.

I think this optimization for cache.put() in page is out of scope of the first launch.
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea7edb72bfce8b570be655c14590a8b68f61635d

commit ea7edb72bfce8b570be655c14590a8b68f61635d
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Sun Nov 12 04:14:00 2017

Introduce PWAFullCodeCache flag

We will implement this feature behind the flag.

BUG= 768705 

Change-Id: Ie8a75a3db722fefe14c8ea89a6b23378282f4d00
Reviewed-on: https://chromium-review.googlesource.com/761768
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515860}
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/chrome/browser/about_flags.cc
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/content/child/runtime_features.cc
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/content/public/common/content_features.cc
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/content/public/common/content_features.h
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/third_party/WebKit/Source/platform/exported/WebRuntimeFeatures.cpp
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/third_party/WebKit/Source/platform/runtime_enabled_features.json5
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/third_party/WebKit/public/platform/WebRuntimeFeatures.h
[modify] https://crrev.com/ea7edb72bfce8b570be655c14590a8b68f61635d/tools/metrics/histograms/enums.xml

Comment 18 by horo@chromium.org, Nov 12 2017

Status: Started (was: Assigned)

Comment 19 by horo@chromium.org, Nov 15 2017 

Ah, I set wrong BUG ID..

---------------
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/faf9a296f716358e3ac6ab3f5d2b0a304f99f517

commit faf9a296f716358e3ac6ab3f5d2b0a304f99f517
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Nov 15 16:11:44 2017

Add UMA for CacheStorage installed scripts

This CL introduces these UMAs:
  ServiceWorker.CacheStorageInstalledScript.ScriptSize
  ServiceWorker.CacheStorageInstalledScript.Count
  ServiceWorker.CacheStorageInstalledScript.ScriptTotalSize

Bug:  784018 
Change-Id: Idf6c93b527edae077ce122bc825b7513cf7303fb
Reviewed-on: https://chromium-review.googlesource.com/771172
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516710}
[modify] https://crrev.com/faf9a296f716358e3ac6ab3f5d2b0a304f99f517/third_party/WebKit/Source/modules/cachestorage/Cache.cpp
[modify] https://crrev.com/faf9a296f716358e3ac6ab3f5d2b0a304f99f517/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
[modify] https://crrev.com/faf9a296f716358e3ac6ab3f5d2b0a304f99f517/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.h
[modify] https://crrev.com/faf9a296f716358e3ac6ab3f5d2b0a304f99f517/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeProxy.cpp
[modify] https://crrev.com/faf9a296f716358e3ac6ab3f5d2b0a304f99f517/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
[modify] https://crrev.com/faf9a296f716358e3ac6ab3f5d2b0a304f99f517/tools/metrics/histograms/histograms.xml
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e03207600c9a0c6fef880c6996ad3edb59f7abb

commit 8e03207600c9a0c6fef880c6996ad3edb59f7abb
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Thu Nov 16 00:09:21 2017

Add RendererMemoryTiming UMA for ServiceWorkerControlledMainFrames

We want to use these metrics to measure the memory impact of
PWAFullCodeCache.

BUG= 768705 

Change-Id: I1d3f669d0aca6ebd544ed6a05a0bb373a3fa2ddf
Reviewed-on: https://chromium-review.googlesource.com/771110
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516908}
[modify] https://crrev.com/8e03207600c9a0c6fef880c6996ad3edb59f7abb/base/metrics/histogram_functions.cc
[modify] https://crrev.com/8e03207600c9a0c6fef880c6996ad3edb59f7abb/base/metrics/histogram_functions.h
[modify] https://crrev.com/8e03207600c9a0c6fef880c6996ad3edb59f7abb/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/8e03207600c9a0c6fef880c6996ad3edb59f7abb/content/renderer/render_frame_impl.h
[modify] https://crrev.com/8e03207600c9a0c6fef880c6996ad3edb59f7abb/tools/metrics/histograms/histograms.xml
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4191bbf4caa1a3134f65c292909be9b3a1fa35a

commit d4191bbf4caa1a3134f65c292909be9b3a1fa35a
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Thu Nov 16 07:03:09 2017

Support side data in CacheStorageCache.Put

We will use the side data of CacheStorageCache.Put to store the
script code cache while installing service worker.

BUG= 768705 

Change-Id: I84b2491bde1f1230b532ce12bf8573be905b59fd
Reviewed-on: https://chromium-review.googlesource.com/768341
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517006}
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/bad_message.h
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/cache_storage/cache_storage_cache.h
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/cache_storage/cache_storage_cache_unittest.cc
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/cache_storage/cache_storage_dispatcher_host.h
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/browser/cache_storage/cache_storage_manager_unittest.cc
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/common/service_worker/service_worker_messages.h
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/common/service_worker/service_worker_types.cc
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/common/service_worker/service_worker_types.h
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/content/renderer/service_worker/service_worker_type_util.cc
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/third_party/WebKit/Source/platform/exported/WebServiceWorkerResponse.cpp
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponse.h
[modify] https://crrev.com/d4191bbf4caa1a3134f65c292909be9b3a1fa35a/tools/metrics/histograms/enums.xml
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5

commit bd3ab3b7c568a6c5c2738260f3a35829631dd8a5
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Nov 21 01:18:46 2017

Generates V8 full code cache in Cache.Put while installing service worker.

Bug:  768705 
Change-Id: I0b64a5c545980cb49405ab5a60217dda0ac48c3a
Reviewed-on: https://chromium-review.googlesource.com/737595
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518009}
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/VirtualTestSuites
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/resources/v8-cache-iframe.html
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/resources/v8-cache-worker.js
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/service-worker-v8-cache-expected.txt
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/service-worker-v8-cache.js
[add] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/virtual/pwa-full-code-cache/http/tests/devtools/service-workers/README.txt
[add] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/LayoutTests/virtual/pwa-full-code-cache/http/tests/devtools/service-workers/service-worker-v8-cache-expected.txt
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/devtools/front_end/performance_test_runner/TimelineTestRunner.js
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/modules/cachestorage/Cache.cpp
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/modules/cachestorage/Cache.h
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.h
[modify] https://crrev.com/bd3ab3b7c568a6c5c2738260f3a35829631dd8a5/tools/metrics/histograms/histograms.xml
Project Member

Comment 23 by bugdroid1@chromium.org, Nov 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b24a111a60e8baca13a1176fff468aec98cc7c44

commit b24a111a60e8baca13a1176fff468aec98cc7c44
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Nov 22 02:20:22 2017

DevTools: Suspend a new target when TargetManager._isSuspended is true

When DevTools start recording the performance timeline, TargetManager suspends
the all targets. But if a new target is added by createTarget, currently the new
target is not suspended.

This is problematic while measuring the performance of PWAFullCodeCache, because
V8 doesn't generate code cache if debugger is attached to the newly started
service worker.

Bug:  768705 
Change-Id: Ib54f89123ce96187fcbcc390ba2713c00465abbb
Reviewed-on: https://chromium-review.googlesource.com/778706
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518506}
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/resources/v8-cache-worker.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/service-worker-v8-cache-expected.txt
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/service-worker-v8-cache.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/LayoutTests/virtual/pwa-full-code-cache/http/tests/devtools/service-workers/service-worker-v8-cache-expected.txt
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/Source/devtools/front_end/sdk/OverlayModel.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/Source/devtools/front_end/sdk/Target.js
[modify] https://crrev.com/b24a111a60e8baca13a1176fff468aec98cc7c44/third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js

Comment 24 by horo@chromium.org, Nov 22 2017

Blockedon: 787736
Project Member

Comment 25 by bugdroid1@chromium.org, Nov 24 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bcb1c649cef95651a958437ece5bf805f1f74079

commit bcb1c649cef95651a958437ece5bf805f1f74079
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Fri Nov 24 16:32:14 2017

Introduce ServiceWorkerScriptFullCodeCache feature

When this feature is enabled, V8 full code cache of service worker
scripts are created while installing the service worker.

Bug:  768705 
Change-Id: If5942b1d9988098fd95e2473a67924b8a271b607
Reviewed-on: https://chromium-review.googlesource.com/784658
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519130}
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/chrome/browser/about_flags.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/browser/service_worker/service_worker_version_unittest.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/child/runtime_features.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/public/common/content_features.cc
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/content/public/common/content_features.h
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/testing/variations/fieldtrial_testing_config.json
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/Source/bindings/core/v8/V8CacheOptions.h
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/Source/platform/exported/WebRuntimeFeatures.cpp
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/Source/platform/runtime_enabled_features.json5
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/third_party/WebKit/public/platform/WebRuntimeFeatures.h
[modify] https://crrev.com/bcb1c649cef95651a958437ece5bf805f1f74079/tools/metrics/histograms/enums.xml
Project Member

Comment 26 by bugdroid1@chromium.org, Nov 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9cdaa227686d4e995569173d451cab0767ac48f

commit e9cdaa227686d4e995569173d451cab0767ac48f
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Mon Nov 27 09:57:08 2017

Add PWAFullCodeCache to fieldtrial_testing_config.json

Bug:  768705 
Change-Id: Ifb6cb194d85e34264d7fb8071f12df4ecddad962
Reviewed-on: https://chromium-review.googlesource.com/781321
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519261}
[modify] https://crrev.com/e9cdaa227686d4e995569173d451cab0767ac48f/testing/variations/fieldtrial_testing_config.json

Comment 27 by falken@chromium.org, Jan 17 2018 

This came up in a meeting. horo@ do we have any results we can share from the experiment so far?

Comment 28 by horo@chromium.org, Jan 19 2018 

We did an experiment in Dev/Canary.
But it is difficult to find a clear evidence of the performance improvement of this feature.

I think this is mainly because of the high frequency of Chrome version update.
The code cache is not used after the V8 version update.

So I'm planning to start the experiment in Beta and Stable.

Comment 29 by falken@chromium.org, Mar 13 2018 

This came up in a meeting. What's the current status of the experiment rollout and are there any results we can share?

Also did we record anything about the decision to do this as an origin trial/not?

Comment 30 by horo@chromium.org, Mar 14 2018 

We just started the experiment in M65 stable which was released last week.
We are waiting for the stable version to be widely rolled out.

I think we didn't do the OriginTrial because these features don't have any Web API.

Comment 31 by yangguo@chromium.org, Mar 26 2018 

See https://bugs.chromium.org/p/v8/issues/detail?id=7591

The result of that bug is that this experiment is not testing what we think it is testing.

Comment 32 by slightlyoff@chromium.org, Apr 3 2018 

65 has been out a while; do we have data yet?

Comment 33 by yangguo@chromium.org, Apr 3 2018 

Btw disregard comment 31. It was a false alarm due to some misplaced assertions.
Project Member

Comment 34 by bugdroid1@chromium.org, Apr 4 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1f3717e6f14cbbe9400d2b2295b78719b5d8d37

commit c1f3717e6f14cbbe9400d2b2295b78719b5d8d37
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Apr 04 01:42:28 2018

Enable PWAFullCodeCache by default

Bug:  768705 ,788621
Change-Id: I0e0b2eb03da7c6c65e871db94c836ac4c06a46e6
Reviewed-on: https://chromium-review.googlesource.com/994493
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547930}
[modify] https://crrev.com/c1f3717e6f14cbbe9400d2b2295b78719b5d8d37/content/public/common/content_features.cc
Project Member

Comment 35 by bugdroid1@chromium.org, Apr 4 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4334bc5b70e16494bad3511562a045f5ccb70a7

commit c4334bc5b70e16494bad3511562a045f5ccb70a7
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Apr 04 02:02:48 2018

Enable ServiceWorkerScriptFullCodeCache by default

Bug:  768705 ,788619
Change-Id: I18474261a0bb443b5ed7186adb8bc68227ece26c
Reviewed-on: https://chromium-review.googlesource.com/994492
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547946}
[modify] https://crrev.com/c4334bc5b70e16494bad3511562a045f5ccb70a7/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/c4334bc5b70e16494bad3511562a045f5ccb70a7/content/public/common/content_features.cc

Comment 36 by horo@chromium.org, Apr 10 2018

Status: Fixed (was: Started)
I enabled these features (PWAFullCodeCache, ServiceWorkerScriptFullCodeCache) by default for all channels (Canary/Dev/Beta/Stable) >= M65 using Chrome Variations framework now.
Project Member

Comment 37 by bugdroid1@chromium.org, Apr 17 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5

commit b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Apr 17 05:44:17 2018

Revert "Enable ServiceWorkerScriptFullCodeCache by default"

This reverts commit c4334bc5b70e16494bad3511562a045f5ccb70a7.

Reason for revert: Caused crashes when storing javascript modules in cache storage.  https://crbug.com/832202 

Original change's description:
> Enable ServiceWorkerScriptFullCodeCache by default
> 
> Bug:  768705 ,788619
> Change-Id: I18474261a0bb443b5ed7186adb8bc68227ece26c
> Reviewed-on: https://chromium-review.googlesource.com/994492
> Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547946}

TBR=horo@chromium.org,kinuko@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  768705 , 788619,  832202 
Change-Id: I53d3a401dfb83fbf54184588ca889b1a4f9f0474
Reviewed-on: https://chromium-review.googlesource.com/1014310
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551258}
[modify] https://crrev.com/b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5/content/public/common/content_features.cc
Project Member

Comment 38 by bugdroid1@chromium.org, Apr 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c8633b7d44e30d1d48553eeb545884c45c9af21c

commit c8633b7d44e30d1d48553eeb545884c45c9af21c
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Thu Apr 19 00:31:31 2018

Reland "Enable ServiceWorkerScriptFullCodeCache by default"

This reverts commit b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5.

Reason for revert: The CL (crrev.com/c/1014744) which fixes the crash bug landed.

Original change's description:
> Revert "Enable ServiceWorkerScriptFullCodeCache by default"
> 
> This reverts commit c4334bc5b70e16494bad3511562a045f5ccb70a7.
> 
> Reason for revert: Caused crashes when storing javascript modules in cache storage.  https://crbug.com/832202 
> 
> Original change's description:
> > Enable ServiceWorkerScriptFullCodeCache by default
> > 
> > Bug:  768705 ,788619
> > Change-Id: I18474261a0bb443b5ed7186adb8bc68227ece26c
> > Reviewed-on: https://chromium-review.googlesource.com/994492
> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#547946}
> 
> TBR=horo@chromium.org,kinuko@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  768705 , 788619,  832202 
> Change-Id: I53d3a401dfb83fbf54184588ca889b1a4f9f0474
> Reviewed-on: https://chromium-review.googlesource.com/1014310
> Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551258}

TBR=horo@chromium.org,kinuko@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  768705 , 788619,  832202 
Change-Id: I052ec2e159627facff21dda99f3fbfe15326aa6a
Reviewed-on: https://chromium-review.googlesource.com/1018044
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551897}
[modify] https://crrev.com/c8633b7d44e30d1d48553eeb545884c45c9af21c/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/c8633b7d44e30d1d48553eeb545884c45c9af21c/content/public/common/content_features.cc

Comment 39 by horo@chromium.org, Apr 19 2018 

I disabled PWAFullCodeCache for all channels (Canary/Dev/Beta/Stable) using Chrome Variations framework due to the crash bug ( crbug.com/832202 ).
The fix CL (c46ccbae6b623bea947f4cd5c92c19961c20f372) for the crash bug landed in 68.0.3400.0.

After I will merge the CL to M67 ( crbug.com/832202#c22 ), I will enable PWAFullCodeCache for all channels >= M67.

Comment 40 by horo@chromium.org, Apr 20 2018 

The fix CL (c46ccbae6b623bea947f4cd5c92c19961c20f372) was merged to 67.0.3396.13 as 7336b1875146492d1c7c41c4bcf63e7f851b5410.
I enabled PWAFullCodeCache for all channels >= 67.0.3396.13.

Comment 41 by falken@chromium.org, Jul 24 

(archaeology) If I understand correctly, PWAFullCodeCache was disabled via server-side config on M65 and M66 due to  issue 832202 . It was enabled it by default in M67 as per comment 40. See also issue 788621.

I think ServiceWorkerScriptFullCodeCache remained shipping via since M65.
Project Member

Comment 42 by bugdroid1@chromium.org, Jul 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e956395ba71a3d226ce0b163dbe009623a02ebe

commit 7e956395ba71a3d226ce0b163dbe009623a02ebe
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jul 24 22:26:31 2018

Remove obsolete field trial testing configs for service worker experiments.

These have all been enabled by default client-side or removed for some time:
- PWAFullCodeCache in Chrome 67
- ServiceWorkerNavigationPreload in Chrome 59
- ServiceWorkerScriptFullCodeCache in Chrome 67
- ServiceWorkerScriptStreaming in Chrome 64
- SpeculativeLaunchServiceWorker was removed

Bug:  661071 ,  683037 ,  768705 , 640132
Change-Id: Id218f3882273063db7e4c358cac956ca54f362d8
Reviewed-on: https://chromium-review.googlesource.com/1147895
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577711}
[modify] https://crrev.com/7e956395ba71a3d226ce0b163dbe009623a02ebe/testing/variations/fieldtrial_testing_config.json
►

Sign in to add a comment