Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3011263002:
Add cache_temperature state "HOT" (Closed)
DescriptionAdd cache_temperature state "HOT"
This CL Adds cache_temperature state "HOT".
There were three states currently: ANY, COLD, WARM.
When testing PWAs, we need three states below.
1st loading: no Service Worker (COLD)
2nd loading: Service Worker installed and not having worker script code cache (WARM)
After 3rd loading: Service Worker installed and having worker script with v8 code cache and stored contents data (HOT)
Current COLD does not clear service worker data.
Also, current WARM does not wait for Service Worker installation to be finished, and does not ensure 2nd loading (but 2nd or more times).
In this CL, cache_temperature warms cache with page.RunNavigateSteps() and wait loading to be completed with page.RunPageInteractions() in order to wait service worker registration.
Additionally, modify ClearCache() to ClearCacheAndData() to clear not only browser cache but also storage data including service worker's.
New state "HOT" and modified state "COLD" and "WARM" are implemented using these.
Design doc of perf benchmark for PWA:
https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yuuo/edit?usp=sharing
BUG=chromium:736697
Review-Url: https://chromiumcodereview.appspot.com/3011263002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/7c166a82feaf1b63353ad60affe506b3374ae56c
Patch Set 1 #Patch Set 2 : testing #Patch Set 3 : add dependency #Patch Set 4 : enable StopServiceWorker() #Patch Set 5 : merged #
Total comments: 4
Patch Set 6 : Call tab methods directly instead of using StopServiceWorker() #Patch Set 7 : use methods for serviceworker in tab #
Total comments: 12
Patch Set 8 : add HotAfterCold test #Patch Set 9 : call tab.StopAllServiceWorkers() after each navigation #
Total comments: 6
Patch Set 10 : add comments #Patch Set 11 : check markers separately #
Total comments: 6
Patch Set 12 : add TODO comment #Patch Set 13 : merge #Patch Set 14 : add decorators #
Dependent Patchsets: Messages
Total messages: 55 (26 generated)
Description was changed from ========== Add cache_temperature state This CL Add (rename) cache_temperature state. There are 4 states: ANY, COLD, WARM, HOT. PCV1_* are used now, but they will be removed in following CLs. Indicating each state, we can run perf tests with each cache state. After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium736697 ========== to ========== Add cache_temperature state This CL Adds (renames) cache_temperature state. There are 4 states: ANY, COLD, WARM, HOT. PCV1_* are used now, but they will be removed in following CLs. Indicating each state, we can run perf tests with each cache state. After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium736697 ==========
Description was changed from ========== Add cache_temperature state This CL Adds (renames) cache_temperature state. There are 4 states: ANY, COLD, WARM, HOT. PCV1_* are used now, but they will be removed in following CLs. Indicating each state, we can run perf tests with each cache state. After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium736697 ========== to ========== Add cache_temperature state This CL Adds (renames) cache_temperature state. There are 4 states: ANY, COLD, WARM, HOT. PCV1_* are used now, but they will be removed in following CLs. Indicating each state, we can run perf tests with each cache state. After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
Description was changed from ========== Add cache_temperature state This CL Adds (renames) cache_temperature state. There are 4 states: ANY, COLD, WARM, HOT. PCV1_* are used now, but they will be removed in following CLs. Indicating each state, we can run perf tests with each cache state. After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add cache_temperature state This CL Adds cache_temperature state "HOT". After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
yukiy@google.com changed reviewers: + kouhei@chromium.org, nednguyen@google.com, shimazu@chromium.org
This adds cache_temperature.HOT and uses some methods added in previous CLs. PTAL:D
On 2017/09/19 10:11:24, yukiy wrote: > This adds cache_temperature.HOT and uses some methods added in previous CLs. > PTAL:D I will wait for other CLs to land & this CL is rebased before reviewing it
Please check this CL again!
yukiy@google.com changed reviewers: + falken@chromium.org
https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag... telemetry/telemetry/page/cache_temperature.py:71: def StopServiceWorker(browser, timeout=60): s/StopServiceWorker/InstallServiceWorker https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag... telemetry/telemetry/page/cache_temperature.py:73: res = tab._inspector_backend._websocket.SyncRequest({ Please don't access private APIs at inspector backend or websocket level here. All these calls should look like: tab.EnableServiceWorker() tab.StopServiceWorker()
PTAL https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag... telemetry/telemetry/page/cache_temperature.py:71: def StopServiceWorker(browser, timeout=60): On 2017/09/22 08:35:14, nednguyen wrote: > s/StopServiceWorker/InstallServiceWorker Acknowledged. https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag... telemetry/telemetry/page/cache_temperature.py:73: res = tab._inspector_backend._websocket.SyncRequest({ On 2017/09/22 08:35:14, nednguyen wrote: > Please don't access private APIs at inspector backend or websocket level here. > All these calls should look like: > > tab.EnableServiceWorker() > tab.StopServiceWorker() OK! I created new CL so please check it :) https://codereview.chromium.org/3018603002/
https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:102: WarmCache(page, browser) I'm wondering if it's correct though I'm not familiar with the problem of double hash navigation. Do we need to navigate to does.not.exist before each Navigate() if there is '#' in the page.url? If it's correct, I guess we need to put avoid_double_hash_navigation logic here, but it seems awkward... I guess tab.Navigate or something might be a good place to put the pre-navigation mechanism. kouhei/ned: Do you have any ideas about it? https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:121: WarmCache(page, browser) We need to stop workers for each WarmCache() since the number of script evaluations for the service worker must be three or more when HOT temperature/two for WARM to ensure existence of V8 code cache. https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature_unittest.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:127: Can you make a test to run HOT after COLD? https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:139: self.assertIn('telemetry.internal.warm_cache.start', markers) Should we expect two telemetry.internal.warm_cache.start in markers?
On 2017/09/25 02:30:36, shimazu wrote: > https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... > File telemetry/telemetry/page/cache_temperature.py (right): > > https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... > telemetry/telemetry/page/cache_temperature.py:102: WarmCache(page, browser) > I'm wondering if it's correct though I'm not familiar with the problem of double > hash navigation. > Do we need to navigate to does.not.exist before each Navigate() if there is '#' > in the page.url? If it's correct, I guess we need to put > avoid_double_hash_navigation logic here, but it seems awkward... > > I guess tab.Navigate or something might be a good place to put the > pre-navigation mechanism. > kouhei/ned: Do you have any ideas about it? > kouhei@ mentioned it at https://codereview.chromium.org/3013213002/ https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca... telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url: Would you mind keeping AvoidDoubleHashNavigation as is? Having this here in EnsurePageCacheTemperature is a *hack* and I think this should ideally live in tab.py Navigate. Factoring out feels like making the hack permanent.
https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature_unittest.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:139: self.assertIn('telemetry.internal.warm_cache.start', markers) self.assertEqual(2, markers.count('telemetry.internal.warm_cache.start'))
https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:102: WarmCache(page, browser) Copied the reply from yukiy@: > > kouhei@ mentioned it at https://codereview.chromium.org/3013213002/ > > https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca... > telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url: > Would you mind keeping AvoidDoubleHashNavigation as is? > Having this here in EnsurePageCacheTemperature is a *hack* and I think this > should ideally live in tab.py Navigate. Factoring out feels like making the hack > permanent. Yeah, but if we need to call it after all of WarmCache(), I think it'd be better to have it as a function and put a TODO comment.
PTAL https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:121: WarmCache(page, browser) On 2017/09/25 02:30:35, shimazu wrote: > We need to stop workers for each WarmCache() since the number of script > evaluations for the service worker must be three or more when HOT > temperature/two for WARM to ensure existence of V8 code cache. I will do this with avoid_double_hash_navigation :D https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature_unittest.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:127: On 2017/09/25 02:30:36, shimazu wrote: > Can you make a test to run HOT after COLD? Done. https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:139: self.assertIn('telemetry.internal.warm_cache.start', markers) On 2017/09/25 02:30:36, shimazu wrote: > Should we expect two telemetry.internal.warm_cache.start in markers? Done. https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:139: self.assertIn('telemetry.internal.warm_cache.start', markers) On 2017/09/25 05:29:53, kouhei (in TOK) wrote: > self.assertEqual(2, markers.count('telemetry.internal.warm_cache.start')) Acknowledged.
Description was changed from ========== Add cache_temperature state This CL Adds cache_temperature state "HOT". After this CL is committed, cache_temperature will take service worker cache/data states in account too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add cache_temperature state "HOT" This CL Adds cache_temperature state "HOT". There were three states currently: ANY, COLD, WARM. When testing PWAs, we need three states below. 1st loading: no Service Worker (COLD) 2nd loading: Service Worker installed and not having worker script code cache (WARM) After 3rd loading: Service Worker installed and having worker script with v8 code cache and stored contents data (HOT) Current COLD does not clear service worker data. Also, current WARM does not wait for Service Worker installation to be finished, and does not ensure 2nd loading (but 2nd or more times). In this CL, cache_temperature warms cache with page.RunNavigateSteps() and wait loading to be completed with page.RunPageInteractions() in order to wait service worker registration. Additionally, modify ClearCache() to ClearCacheAndData() to clear not only browser cache but also storage data including service worker's. New state "HOT" and modified state "COLD" and "WARM" are implemented using these. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
I modified issue description, so please check it too. PTAL;-) https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:102: WarmCache(page, browser) On 2017/09/25 05:36:17, shimazu wrote: > Copied the reply from yukiy@: > > > > kouhei@ mentioned it at https://codereview.chromium.org/3013213002/ > > > > > https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca... > > telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url: > > Would you mind keeping AvoidDoubleHashNavigation as is? > > Having this here in EnsurePageCacheTemperature is a *hack* and I think this > > should ideally live in tab.py Navigate. Factoring out feels like making the > hack > > permanent. > > Yeah, but if we need to call it after all of WarmCache(), I think it'd be better > to have it as a function and put a TODO comment. As we chatted, avoid_double_hash_navigation logic avoids completing the navigation with only scrolling the page. WarmCache() navigates to about:blank after main navigation, so calling this logic is not needed. https://codereview.chromium.org/3011263002/diff/120001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:121: WarmCache(page, browser) On 2017/09/25 06:28:37, yukiy wrote: > On 2017/09/25 02:30:35, shimazu wrote: > > We need to stop workers for each WarmCache() since the number of script > > evaluations for the service worker must be three or more when HOT > > temperature/two for WARM to ensure existence of V8 code cache. > > I will do this with avoid_double_hash_navigation :D Done.
lgtm I think it's better to wrap the description by 80 characters (though there is no rule for it).
https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:101: browser.tabs[0].StopAllServiceWorkers() Would you add a comment why this is needed? https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature_unittest.py (right): https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:54: all('telemetry.internal.warm_cache' not in maker for maker in markers)) s/maker/marker/ why not assertNotIn? https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:88: all('telemetry.internal.warm_cache' not in maker for maker in markers)) ditto https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:129: all('telemetry.internal.warm_cache' not in maker for maker in markers)) ditto
PTAL https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:101: browser.tabs[0].StopAllServiceWorkers() On 2017/09/26 04:40:52, kouhei (in TOK) wrote: > Would you add a comment why this is needed? Done. Also added a comment for StopAllServiceWorkers() in WarmCache(). https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature_unittest.py (right): https://codereview.chromium.org/3011263002/diff/160001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature_unittest.py:54: all('telemetry.internal.warm_cache' not in maker for maker in markers)) On 2017/09/26 04:40:52, kouhei (in TOK) wrote: > s/maker/marker/ > why not assertNotIn? Because I added temperature for identifier in cache_temperature.py as: with MarkTelemetryInternal(browser, 'warm_cache.%s' % temperature): I want to ensure that there are two "warm_cache" in |markers| at testEnsureHotFromScratch(), but this |markers| is not a list but a set, so I have to add some tags to check multiple "warm_cache". Are there any other better ways?
kouhei@, as we chatted offline, I made separate assertion. PTAL
lgtm
https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:127: browser.tabs[0].StopAllServiceWorkers() I defer to kouhei and ned for the owner review. But this nested if/else plus duplicated comments+code is a bit difficult to read. I'd suggest at least splitting lines 116-127 into common helper function(s)?
PTAL https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:127: browser.tabs[0].StopAllServiceWorkers() On 2017/09/26 08:46:06, falken wrote: > I defer to kouhei and ned for the owner review. But this nested if/else plus > duplicated comments+code is a bit difficult to read. I'd suggest at least > splitting lines 116-127 into common helper function(s)? Sorry for confusing you, but it was discussed in other CL: https://codereview.chromium.org/3013213002/ > telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url: > Would you mind keeping AvoidDoubleHashNavigation as is? > Having this here in EnsurePageCacheTemperature is a *hack* and I think this > should ideally live in tab.py Navigate. Factoring out feels like making the hack > permanent.
https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:64: page.RunNavigateSteps(tab.action_runner) wait, you mean page.RunNavigateSteps(page.url), right? You cannot navigate to tab.action_runner, which is an object, not a url string. https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:127: browser.tabs[0].StopAllServiceWorkers() On 2017/09/26 09:01:32, yukiy wrote: > On 2017/09/26 08:46:06, falken wrote: > > I defer to kouhei and ned for the owner review. But this nested if/else plus > > duplicated comments+code is a bit difficult to read. I'd suggest at least > > splitting lines 116-127 into common helper function(s)? > > Sorry for confusing you, but it was discussed in other CL: > https://codereview.chromium.org/3013213002/ > > > telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url: > > Would you mind keeping AvoidDoubleHashNavigation as is? > > Having this here in EnsurePageCacheTemperature is a *hack* and I think this > > should ideally live in tab.py Navigate. Factoring out feels like making the > hack > > permanent. shimazu@: can you pick up the refactoring work after this? I agree with falken is this code is too difficult to read.
PTAL! https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:64: page.RunNavigateSteps(tab.action_runner) On 2017/09/26 10:07:19, nednguyen wrote: > wait, you mean page.RunNavigateSteps(page.url), right? You cannot navigate to > tab.action_runner, which is an object, not a url string. In loading.mobile, it will be PageCyclerStory.RunNavigateSteps(action_runner). https://cs.chromium.org/chromium/src/tools/perf/page_sets/loading_mobile.py?d... I changed it in other CL: https://chromium-review.googlesource.com/c/chromium/src/+/647128 Also page.RunNavigateSteps(action_runner) is called in unittest. https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
I would want a bug to improve the readability of code to setup the cache state before stamping this https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... File telemetry/telemetry/page/cache_temperature.py (right): https://codereview.chromium.org/3011263002/diff/200001/telemetry/telemetry/pa... telemetry/telemetry/page/cache_temperature.py:64: page.RunNavigateSteps(tab.action_runner) On 2017/09/26 10:36:25, yukiy wrote: > On 2017/09/26 10:07:19, nednguyen wrote: > > wait, you mean page.RunNavigateSteps(page.url), right? You cannot navigate to > > tab.action_runner, which is an object, not a url string. > > In loading.mobile, it will be PageCyclerStory.RunNavigateSteps(action_runner). > https://cs.chromium.org/chromium/src/tools/perf/page_sets/loading_mobile.py?d... > I changed it in other CL: > https://chromium-review.googlesource.com/c/chromium/src/+/647128 > > Also page.RunNavigateSteps(action_runner) is called in unittest. > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... ok
On 2017/09/26 10:49:04, nednguyen wrote: > I would want a bug to improve the readability of code to setup the cache state > before stamping this https://crbug.com/768780 Is this looks OK? If so, I will add this bug to TODO comment:D
On 2017/09/26 11:03:01, yukiy wrote: > On 2017/09/26 10:49:04, nednguyen wrote: > > I would want a bug to improve the readability of code to setup the cache state > > before stamping this > > https://crbug.com/768780 > Is this looks OK? > If so, I will add this bug to TODO comment:D look ok to me lgtm
On 2017/09/26 11:12:13, nednguyen wrote: > On 2017/09/26 11:03:01, yukiy wrote: > > On 2017/09/26 10:49:04, nednguyen wrote: > > > I would want a bug to improve the readability of code to setup the cache > state > > > before stamping this > > > > https://crbug.com/768780 > > Is this looks OK? > > If so, I will add this bug to TODO comment:D > > look ok to me > > lgtm Thanks so much! I will commit this;-)
Patchset #12 (id:220001) has been deleted
The CQ bit was checked by yukiy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, nednguyen@google.com, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/3011263002/#ps240001 (title: "add TODO comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
The CQ bit was checked by yukiy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
The CQ bit was checked by yukiy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Try jobs can pass after committed this CL: https://codereview.chromium.org/3013263002/ Please check it:D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yukiy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yukiy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, shimazu@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/3011263002/#ps280001 (title: "add decorators")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1506506046817940,
"parent_rev": "0ce28daf74ebff5a087ccda7db9d6bcfc77dfdf6", "commit_rev":
"7c166a82feaf1b63353ad60affe506b3374ae56c"}
Message was sent while issue was closed.
Description was changed from ========== Add cache_temperature state "HOT" This CL Adds cache_temperature state "HOT". There were three states currently: ANY, COLD, WARM. When testing PWAs, we need three states below. 1st loading: no Service Worker (COLD) 2nd loading: Service Worker installed and not having worker script code cache (WARM) After 3rd loading: Service Worker installed and having worker script with v8 code cache and stored contents data (HOT) Current COLD does not clear service worker data. Also, current WARM does not wait for Service Worker installation to be finished, and does not ensure 2nd loading (but 2nd or more times). In this CL, cache_temperature warms cache with page.RunNavigateSteps() and wait loading to be completed with page.RunPageInteractions() in order to wait service worker registration. Additionally, modify ClearCache() to ClearCacheAndData() to clear not only browser cache but also storage data including service worker's. New state "HOT" and modified state "COLD" and "WARM" are implemented using these. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add cache_temperature state "HOT" This CL Adds cache_temperature state "HOT". There were three states currently: ANY, COLD, WARM. When testing PWAs, we need three states below. 1st loading: no Service Worker (COLD) 2nd loading: Service Worker installed and not having worker script code cache (WARM) After 3rd loading: Service Worker installed and having worker script with v8 code cache and stored contents data (HOT) Current COLD does not clear service worker data. Also, current WARM does not wait for Service Worker installation to be finished, and does not ensure 2nd loading (but 2nd or more times). In this CL, cache_temperature warms cache with page.RunNavigateSteps() and wait loading to be completed with page.RunPageInteractions() in order to wait service worker registration. Additionally, modify ClearCache() to ClearCacheAndData() to clear not only browser cache but also storage data including service worker's. New state "HOT" and modified state "COLD" and "WARM" are implemented using these. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 Review-Url: https://chromiumcodereview.appspot.com/3011263002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
