Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501353002:
Parallelize disk_cache_perftest.
DescriptionParallelize disk_cache_perftest.
Better replicates real browser for testing purposes.
BUG=None
R=jkarlin@chromium.org
Patch Set 1 #Patch Set 2 : now read #Patch Set 3 : moar reasonable parameters #Patch Set 4 : rebase #Patch Set 5 : rebase redux #Patch Set 6 : closer #Patch Set 7 : rebase, again #Patch Set 8 : cleaner #Patch Set 9 : clean up constants #Patch Set 10 : compile better? #
Total comments: 8
Patch Set 11 : compile & simple stuff #Patch Set 12 : rebased post upstream landing #
Total comments: 9
Patch Set 13 : rebase to bring back to life #Messages
Total messages: 44 (36 generated)
The CQ bit was checked by gavinp@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by gavinp@chromium.org 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by gavinp@chromium.org 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gavinp@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gavinp@chromium.org 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...
Description was changed from ========== **INPROGRESS** Parallelize disk_cache_perftest. Better replicates real browser for testing purposes. BUG=None ========== to ========== **INPROGRESS** Parallelize disk_cache_perftest. Better replicates real browser for testing purposes. BUG=None R=jkarlin@chromium.org ==========
gavinp@chromium.org changed reviewers: + jkarlin@chromium.org
Description was changed from ========== **INPROGRESS** Parallelize disk_cache_perftest. Better replicates real browser for testing purposes. BUG=None R=jkarlin@chromium.org ========== to ========== Parallelize disk_cache_perftest. Better replicates real browser for testing purposes. BUG=None R=jkarlin@chromium.org ==========
jkarlin: PTAL. This much more closely replicates a real browser test case, the perf test is a lot more useful with this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gavinp@chromium.org 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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... File net/disk_cache/disk_cache_perftest.cc (right): https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:42: const int kChunkSize = 64 * 1024; Is this based on our actual write size? https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:43: const int kMaxParallelOperations = 8; 10 is what the ResourceScheduler maxes at per tab. Note that H2/QUIC don't limit at all, but they will soon, and will also be 10. https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:114: class WriteHandler { Please add some docs for this class. https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:153: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:281: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:311: disk_cache::Entry* entry = *(entry_ptr.get()); does *entry_ptr work instead of *(entry_ptr.get())? https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:313: EXPECT_EQ(data_len, entry->GetDataSize(1)); Please use an enum or constant for the streams in this file. https://codereview.chromium.org/2501353002/diff/180001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:376: entry.data_len = base::RandInt(0, kBodySize); We have a very good idea of body sizes read from the cache, see Net.HttpJob.PrefilterBytesRead.Cache in dev channel. Let's pull random samples from a log-normal distribution that matches the UMA.
Can you add a BUG= value?
The CQ bit was checked by gavinp@chromium.org 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.
morlovich@chromium.org changed reviewers: + morlovich@chromium.org
I really, really, like this, just mostly want to argue about the constants. Only merge issue was a trivial conflict with https://codereview.chromium.org/2879893002/ (which this does need to be rebased on top of to work). https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... File net/disk_cache/disk_cache_perftest.cc (right): https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:39: const int kHeadersSize = 800; This is likely way too small, at least per SimpleCache.Http.HeaderSize; I think because we sometimes put something cert-related in stream 0 which is 4K-ish? It might make sense to approximate the bimodality by just using 2K or something. https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:41: const int kBodySize = 256 * 1024 - 1; This, OTOH, is way too big unless we are optimizing for media cache; for example on my home machine 75% of files were under 32KiB, and this would make the median 128KiB. Luckily there is also a usable public data for this, in HTTP archive, that's bigqueryable: SELECT quantiles(respBodySize, 11) from httparchive.runs.latest_requests: 1 1 2 43 3 263 4 871 5 1849 6 4005 7 7278 8 12378 9 21052 10 41732 11 134769794 The best UMA metric might be Net.HttpContentLengthCacheable or such? (Of course the distribution this uses is also not realistic at all..) https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:44: const int kChunkSize = 64 * 1024; Don't know where it comes from, but it's clearly using 32K ops these days: t=117030 [st=1] SIMPLE_CACHE_ENTRY_READ_CALL --> buf_len = 32768 --> index = 1 --> offset = 0 t=117031 [st=2] SIMPLE_CACHE_ENTRY_READ_CALL --> buf_len = 32768 --> index = 1 --> offset = 32768 https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:112: int pending_result_; These 3 fields are probably dead? https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:167: EXPECT_GT(kNumEntries, next_entry_index_); ASSERT_GT, given the out-of-bounds access if this fails? https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:222: write_size, callback, false); , true), since I think HttpCache uses truncating writes consistently? https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:295: EXPECT_GT(kNumEntries, next_entry_index_); ASSERT_GT? https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:389: new WriteHandler(this, cache_.get(), cb.callback())); Just allocate on stack? Heap allocation seems pointless here. https://codereview.chromium.org/2501353002/diff/220001/net/disk_cache/disk_ca... net/disk_cache/disk_cache_perftest.cc:400: new ReadHandler(this, what_to_read, cache_.get(), cb.callback())); Likewise.
Some purely practical comments re:benchmark, that aren't directly related to this CL, but I feel like I should write down: 1) It seems like the cache eviction ops on Windows are actually async --- I get an order-of-magnitude variation in "cold" runs unless I put in a sleep(10 sec) after the OS cache flushing stuff in ResetAndEvictSystemDiskCache 2) Getting some trouble with jumpyness on Android as well, which seems helped by bumping up # of iterations --- I am wondering whether it's a matter of CPU speed scaling.
On 2017/05/26 13:31:11, Maks Orlovich wrote: > Some purely practical comments re:benchmark, that aren't directly related to > this CL, but I feel like I should write down: > 1) It seems like the cache eviction ops on Windows are actually async --- I get > an order-of-magnitude variation in > "cold" runs unless I put in a sleep(10 sec) after the OS cache flushing stuff > in ResetAndEvictSystemDiskCache > 2) Getting some trouble with jumpyness on Android as well, which seems helped by > bumping up # of iterations --- I am wondering > whether it's a matter of CPU speed scaling. Do you want me to advance this and land it, or do you want to grab it and continue?
> Do you want me to advance this and land it, or do you want to grab it and > continue? It would probably be less awkward if you were to land it, if that's not too much of an imposition on you.
On 2017/08/04 18:20:41, Maks Orlovich wrote: > > Do you want me to advance this and land it, or do you want to grab it and > > continue? > > It would probably be less awkward if you were to land it, if that's not too much > of an imposition on you. Happy to, on it right away.
The CQ bit was checked by gavinp@chromium.org 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. |
