perf(gather-runner): Clear cache selectively per-pass #2156
Conversation
paulirish
force-pushed
the
dontdisablecache
branch
2 times, most recently
from
a6d3b34 to
e61bb62
Compare
| const useThrottling = options.config.useThrottling; | ||
| return Promise.resolve() | ||
| // Clear disk & memory cache if it's a perf run | ||
| .then(_ => resetStorage && recordTrace && useThrottling && driver.cleanBrowserCaches()) |
There was a problem hiding this comment.
this heuristic seems a little magical, but I can't think of a better one. At some point we'll want an escape hatch (for if you want a perf pass but not a trace...e.g. running your own gatherers or in-page metrics), but in theory finishing #1826 would mean you'd be able to set a disableStorageReset per-pass in the config
| }).then(finalUrl => { | ||
| options.url = finalUrl; | ||
| }); | ||
| const resetStorage = !options.flags.disableStorageReset; |
There was a problem hiding this comment.
we usually let options.flags optionally exist, but I don't care much :)
| @@ -242,10 +242,10 @@ describe('GatherRunner', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| it('clears the network cache and origin storage', () => { | |||
| it('clears origin storage', () => { | |||
There was a problem hiding this comment.
do you want to check that the cache gets turned off and back on again?
There was a problem hiding this comment.
it doesnt go off and on, actually. in the ended didnt go with what the ticket explained. :)
just a one time "Clear everything" at the start of the pass.
added test for that.
There was a problem hiding this comment.
it doesnt go off and on, actually
well I meant whatever this is doing here, then :P
// Toggle 'Disable Cache' to evict the memory cache
.then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true}))
.then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}));There was a problem hiding this comment.
not really. :)
the only thing to really assert is that we want for the first sendCommand's promise to resolve before sending the second. (but technically that's not even necessary)
| const resetStorage = !options.flags.disableStorageReset; | ||
| const recordTrace = options.config.recordTrace; | ||
| const useThrottling = options.config.useThrottling; | ||
| return Promise.resolve() |
There was a problem hiding this comment.
what do you think about moving this to pass? It seems more like setup for navigating (like driver.beginDevtoolsLog and driver.beginTracing) rather than just the act of navigating (putting it here also means going to about:blank also goes through these steps, FWIW)
There was a problem hiding this comment.
yah seems good.
moved beginDevtoolsLog() to be in the promise chain too.
|
ptal |
There was a problem hiding this comment.
lgtm
it'd be nice if we could get more smoke-style tests when we do these protocol-only changes, but I can't think of an obvious, quick way that wouldn't be flaky. In this case and the request blocking case, the unit tests really don't give us much value compared to making sure the commands do what we think they do
Agreed. We've talked before about adding a smokehouse pass that uses completely custom gatherers doing things to verify gatherRunner/driver behavior...we could maybe write one that loads a site, then loads it again in a "perf pass", and checks the network records to make sure it didn't come from the cache...but that might still be flaky. Similar technique could be used to check #2168 |
| const pass = Promise.resolve() | ||
| // Begin tracing only if requested by config. | ||
| // Clear disk & memory cache if it's a perf run | ||
| .then(_ => resetStorage && recordTrace && useThrottling && driver.cleanBrowserCaches()) |
| * i. cleanBrowserCaches() (if it's a perf run) | ||
| * ii. beginDevtoolsLog() | ||
| * iii. beginTrace (if requested) | ||
| * iiv. GatherRunner.loadPage() |
paulirish
force-pushed
the
dontdisablecache
branch
from
e7e3a19 to
ed49797
Compare
Instead of once at the beginning, we now clear on a pass-level, but in reality this is the same as it was the first pass that we need it for.
Additionally, don't leave "disable cache" on. Just clear it and start from empty when necessary.
Cache is cleared if we're on a "perf pass" as defined by
recordTrace && useThrottling.In the name of Operation Yaquina Bay #2146. fixes #2089
From a perf perspective the wins are not as noticeable except on high-end hardware. That's because disk cache isn't really significantly faster than the network. lol. Usually pass #4 drops ~10% in duration (for google news, it was from 900ms total to 800ms total)