Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151283003:
Perf test to measure loading CDM library. (Closed)
|
Created:
6 years, 10 months ago by shadi Modified:
6 years, 9 months ago CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, M-A Ruel Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPerf test to measure loading CDM library.
BUG=337674
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254088
Patch Set 1 #
Total comments: 14
Patch Set 2 : Clean up .gyp. Added clearkeycdm test. #Patch Set 3 : nit #
Total comments: 8
Patch Set 4 : make perf test generic to libraries #
Total comments: 1
Patch Set 5 : add cdm adapter tests #Patch Set 6 : add cdm adapter test cases #
Total comments: 7
Patch Set 7 : Unload library after each load, allow full library names #
Total comments: 4
Patch Set 8 : nits #
Total comments: 8
Patch Set 9 : nits, master rebase #
Total comments: 11
Patch Set 10 : re: thestig@ comments #
Total comments: 1
Patch Set 11 : re: tonyg@ comments #Patch Set 12 : fix windows StringType issue #
Messages
Total messages: 42 (0 generated)
I created a new target that measures the time it takes to load widevinecdm library. We still need to discuss where should this target belong (chrome vs media gyp?), and on which bots to run and how. PTAL https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... File chrome/browser/media/load_cdm_benchmark_test.cc (right): https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:20: base::GetNativeLibraryName(base::ASCIIToUTF16(kWidevineAdapterName))); This does the magic of appending "lib", "dll", "plugin", etc. magic based on platform. I can replace this with kWidevineCdmFileName but then this will be dependent on widevine_cdm_common.h as well.
Thanks. LG overall. Mostly nits. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... File chrome/browser/media/load_cdm_benchmark_test.cc (right): https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:14: const char kWidevineAdapterName[] = "widevinecdm"; This file is specific to WV, but the filename is not, and it's really only line 20 that is. Maybe we can extract a test function and call it with the name of the library we want to load. That would also make it easy to run this on ECK, which could serve as a baseline of the difference between the actual and simplest CDMs. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:16: TEST(CDMBinLoadPerfTest, Basic) { Do we need "Bin"? It seems redundant, but if so, spell out Binary. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:20: base::GetNativeLibraryName(base::ASCIIToUTF16(kWidevineAdapterName))); On 2014/02/01 01:50:07, shadi1 wrote: > This does the magic of appending "lib", "dll", "plugin", etc. magic based on > platform. Coo, maybe we should use this instead of having a different constant for each platform. I guess that's slightly worse performance for production, though. > > I can replace this with kWidevineCdmFileName but then this will be dependent on > widevine_cdm_common.h as well. Shouldn't this file be dependent on widevine_cdm_version.h, which includes widevine_cdm_common.h anyway? This test should not exist if !WIDEVINE_CDM_AVAILABLE, right? We also need to exclude platforms that don't ship the CDM as a library - IOW, run it only when ENABLE_PEPPER_CDMS. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:29: if (!native_library) { How about combining these lines? EXPECT_TRUE(native_library) << "Error loading adapter:\n" << error; Note: adapter is misspelled in the original CL. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:32: perf_test::PrintResult("time_to_load_adapter", I'm not sure how unique this needs to be, but we should probably name it time_to_load_widevine_cdm. (adapter is not really relevant to the metric and there could be other cdm adapters.) Or does l34 make this unique...? In that case: ...s/_adapter/_cdm/. https://codereview.chromium.org/151283003/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (left): https://codereview.chromium.org/151283003/diff/1/chrome/chrome_tests.gypi#old... chrome/chrome_tests.gypi:900: Update the comment. https://codereview.chromium.org/151283003/diff/1/chrome/chrome_tests.gypi#old... chrome/chrome_tests.gypi:901: Should this test really come before browser_tests in this file? The latter seems more common and like it should come earlier. The other perf tests are much farther down in this file. https://codereview.chromium.org/151283003/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/1/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:905: remove these commented items https://codereview.chromium.org/151283003/diff/1/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:907: Do we need GMock?
Thanks PTAL. It seems the .gyp base file in the CL is missing now. So diff does not work. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... File chrome/browser/media/load_cdm_benchmark_test.cc (right): https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:14: const char kWidevineAdapterName[] = "widevinecdm"; On 2014/02/06 19:57:33, ddorwin wrote: > This file is specific to WV, but the filename is not, and it's really only line > 20 that is. > > Maybe we can extract a test function and call it with the name of the library we > want to load. > > That would also make it easy to run this on ECK, which could serve as a baseline > of the difference between the actual and simplest CDMs. Done. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:16: TEST(CDMBinLoadPerfTest, Basic) { On 2014/02/06 19:57:33, ddorwin wrote: > Do we need "Bin"? It seems redundant, but if so, spell out Binary. Done. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:29: if (!native_library) { On 2014/02/06 19:57:33, ddorwin wrote: > How about combining these lines? > EXPECT_TRUE(native_library) << "Error loading adapter:\n" << error; > > Note: adapter is misspelled in the original CL. Done. https://codereview.chromium.org/151283003/diff/1/chrome/browser/media/load_cd... chrome/browser/media/load_cdm_benchmark_test.cc:32: perf_test::PrintResult("time_to_load_adapter", On 2014/02/06 19:57:33, ddorwin wrote: > I'm not sure how unique this needs to be, but we should probably name it > time_to_load_widevine_cdm. (adapter is not really relevant to the metric and > there could be other cdm adapters.) > > Or does l34 make this unique...? In that case: ...s/_adapter/_cdm/. True about the other parameter that makes it unique. I dropped the _cdm from the metric. With current CL the output is something like this: *RESULT time_to_load: widevinecdm= 525.772 ms *RESULT time_to_load: clearkeycdm= .848 ms
LG. A few minor things. https://codereview.chromium.org/151283003/diff/260001/chrome/browser/media/lo... File chrome/browser/media/load_cdm_benchmark_test.cc (right): https://codereview.chromium.org/151283003/diff/260001/chrome/browser/media/lo... chrome/browser/media/load_cdm_benchmark_test.cc:28: perf_test::PrintResult("time_to_load", Just to be safe (in case libraries don't include "cdm"), I suggest adding _cdm to the name. https://codereview.chromium.org/151283003/diff/260001/chrome/browser/media/lo... chrome/browser/media/load_cdm_benchmark_test.cc:35: #if defined(ENABLE_PEPPER_CDMS) around 36-44 https://codereview.chromium.org/151283003/diff/260001/chrome/browser/media/lo... chrome/browser/media/load_cdm_benchmark_test.cc:42: TEST(CDMLoadPerfTest, ClearKey) { External... https://codereview.chromium.org/151283003/diff/260001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/260001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2811: # Executable to measure time to load CDM's. no apostrophe https://codereview.chromium.org/151283003/diff/260001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2821: 'browser/media/load_cdm_benchmark_test.cc', nit: cdm_load in target vs load_cdm in filename - please be consistent. (The test name is CDMLoad...)
Updated the files to be generic. With current changes the output is something like: *RESULT time_to_load_library: libwidevinecdm.so= 557.017 ms *RESULT time_to_load_library: libclearkeycdm.so= .939 ms Also moved file to chrome/browser/ (out of media) https://codereview.chromium.org/151283003/diff/260001/chrome/browser/media/lo... File chrome/browser/media/load_cdm_benchmark_test.cc (right): https://codereview.chromium.org/151283003/diff/260001/chrome/browser/media/lo... chrome/browser/media/load_cdm_benchmark_test.cc:42: TEST(CDMLoadPerfTest, ClearKey) { On 2014/02/07 23:59:12, ddorwin wrote: > External... Done....but the library name does not include external :-/ https://codereview.chromium.org/151283003/diff/260001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/260001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2811: # Executable to measure time to load CDM's. On 2014/02/07 23:59:12, ddorwin wrote: > no apostrophe Done. https://codereview.chromium.org/151283003/diff/260001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2821: 'browser/media/load_cdm_benchmark_test.cc', On 2014/02/07 23:59:12, ddorwin wrote: > nit: cdm_load in target vs load_cdm in filename - please be consistent. (The > test name is CDMLoad...) Done.
lgtm Thanks! Yay! https://codereview.chromium.org/151283003/diff/340001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/340001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:39: MeasureTimeToLoad("widevinecdm"); Should we load the ...adapter? It's the whole package that we really care about in this case. For fun, we could even load both. :)
The CQ bit was checked by shadi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/151283003/440001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
lgtm % nits https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:18: library_full_name = base::FilePath::FromUTF16Unsafe( Can you merge line 17 and 18? https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:25: native_library = base::LoadNativeLibrary(library_path, &error); Merge line 16 into this line? https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:39: MeasureTimeToLoad("widevinecdm"); How about using kWidevineCdmFileName in widevine_cdm_common.h? https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:43: MeasureTimeToLoad("widevinecdmadapter"); Ditto about using kWidevineCdmAdapterFileName.
PTAL (again) on latest change. https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:18: library_full_name = base::FilePath::FromUTF16Unsafe( On 2014/02/08 05:29:38, xhwang wrote: > Can you merge line 17 and 18? Done. https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:25: native_library = base::LoadNativeLibrary(library_path, &error); On 2014/02/08 05:29:38, xhwang wrote: > Merge line 16 into this line? Done. https://codereview.chromium.org/151283003/diff/440001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:39: MeasureTimeToLoad("widevinecdm"); On 2014/02/08 05:29:38, xhwang wrote: > How about using kWidevineCdmFileName in widevine_cdm_common.h? The reason is because kWidevineCdmFileName already includes lib*.so, .dll etc in the name. Then I would have to put base::GetNativeLibraryName() in the TEST() rather being common in MeasureLoadTime() which I want to make. I updated the code to allow both, but not sure if that made it worse :-/ WDYT? Ideally it would be nice to use GetNativeLibraryName() everywhere.
https://codereview.chromium.org/151283003/diff/570001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/570001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:34: void MeasureTimeToLoad(const std::string& short_library_name) { nit: The name should probably be the same as the other function except some reference to ShortName. MeasureTimeToLoadBy/ForShortName? https://codereview.chromium.org/151283003/diff/570001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:57: MeasureTimeToLoad("clearkeycdmadapter"); This string (really the full name) appears in 3 other test files. The MIME type appears in at least a couple. Should we have an external_clear_key_common.h? The biggest question might be where to put it. Probably media/cdm/ if that is reachable from all locations.
https://codereview.chromium.org/151283003/diff/570001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/570001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:34: void MeasureTimeToLoad(const std::string& short_library_name) { On 2014/02/10 21:19:06, ddorwin wrote: > nit: The name should probably be the same as the other function except some > reference to ShortName. MeasureTimeToLoadBy/ForShortName? Done. https://codereview.chromium.org/151283003/diff/570001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:57: MeasureTimeToLoad("clearkeycdmadapter"); On 2014/02/10 21:19:06, ddorwin wrote: > This string (really the full name) appears in 3 other test files. The MIME type > appears in at least a couple. Should we have an external_clear_key_common.h? > > The biggest question might be where to put it. Probably media/cdm/ if that is > reachable from all locations. True, a common file can make things cleaner. But probably outside the scope of this CL.
ddorwin@: any comments? maruel@ for OWNERS LGTM please
On 2014/02/12 02:20:15, shadi1 wrote: > maruel@ for OWNERS LGTM please I'm not sure why you are asking me for OWNERS, since chrome/OWNERS has: per-file *.gypi=* But hey, here's free nit picks! :) https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2812: 'target_name': 'load_library_perf_test', I'm generally against the "add at the bottom of the list" strategy but this file is already completely unsorted. Just noting this fact, nothing to do. https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2818: '../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h', Then this target should not even exist on pure open source checkout.
lgtm % nits. Thanks. https://codereview.chromium.org/151283003/diff/630001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/630001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:35: // name. Check base::GetNativeLibraryName() for details. nit: s/Check/See/ https://codereview.chromium.org/151283003/diff/630001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:36: void MeasureTimeToLoadLibraryByBaseName(const std::string& base_library_name) { nit: MeasureTimeToLoad*Native*LibraryByBaseName https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2818: '../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h', On 2014/02/12 02:32:19, M-A Ruel wrote: > Then this target should not even exist on pure open source checkout. The target exists in an external checkout. There is a dummy widevine_cdm_version_h that is used when branding != "Chrome". This allows us to control the presence solely with branding = "Chrome" and not need to spread the logic throughout the GYP files.
Sorry Maruel, I thought I saw your name on *.gypi= :) Picked thestig@ randomly from list of chrome/* reviewers. thestig@: Can you PTAL? https://codereview.chromium.org/151283003/diff/630001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/630001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:35: // name. Check base::GetNativeLibraryName() for details. On 2014/02/12 18:12:52, ddorwin wrote: > nit: s/Check/See/ Done. https://codereview.chromium.org/151283003/diff/630001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:36: void MeasureTimeToLoadLibraryByBaseName(const std::string& base_library_name) { On 2014/02/12 18:12:52, ddorwin wrote: > nit: MeasureTimeToLoad*Native*LibraryByBaseName Done. https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/630001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2812: 'target_name': 'load_library_perf_test', On 2014/02/12 02:32:19, M-A Ruel wrote: > I'm generally against the "add at the bottom of the list" strategy but this file > is already completely unsorted. Just noting this fact, nothing to do. Yeah, I was not sure if there is any order to follow...
Sorry I misunderstood the per-file *.gypi=* to mean I should pick someone from the list in chrome/OWNERS "Reviewers" section. I guess the meaning of it is that I don't need an OWNER's reviewer for the .gypi file. removing thestig@ and moving maruel@ to cc.
https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:17: EXPECT_TRUE(PathService::Get(base::DIR_MODULE, &output_dir)); I'd ASSERT_TRUE() here so you don't accidentally load the wrong library somehow. https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:18: base::FilePath library_path = output_dir.Append(library_name); Just use AppendASCII() so MeasureTimeToLoadNativeLibraryByBaseName() don't have to do all these utf16 conversions. https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:21: base::NativeLibrary native_library = check this isn't null
https://codereview.chromium.org/151283003/diff/710001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/710001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2818: 'type': '<(gtest_target_type)', Is this going to create another test binary?
thestig@: Thanks for the comments, PTAL, replies inlined. https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/12 20:25:06, Lei Zhang wrote: > nit: no (c) Done. https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:17: EXPECT_TRUE(PathService::Get(base::DIR_MODULE, &output_dir)); On 2014/02/12 20:25:06, Lei Zhang wrote: > I'd ASSERT_TRUE() here so you don't accidentally load the wrong library somehow. Done. https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:18: base::FilePath library_path = output_dir.Append(library_name); On 2014/02/12 20:25:06, Lei Zhang wrote: > Just use AppendASCII() so MeasureTimeToLoadNativeLibraryByBaseName() don't have > to do all these utf16 conversions. I wanted |library_name| to be a FilePath in case the library is within other folders in DIR_MODULE, example inside lib folder. or did I misunderstood you? https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:21: base::NativeLibrary native_library = On 2014/02/12 20:25:06, Lei Zhang wrote: > check this isn't null This was done in l.25 EXPECT_TRUE(native_library) << ... But just realized that we can't UnloadLibrary() if it was null :) I switched to ASSERT and moved it before the UnloadLibrary() call. This won't report any time_to_load if it failed as well. https://codereview.chromium.org/151283003/diff/710001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/710001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2818: 'type': '<(gtest_target_type)', On 2014/02/12 20:26:18, Lei Zhang wrote: > Is this going to create another test binary? Yes, doesn't this create a "load_library_perf_test" binary. The plan is to run it on perf bots to monitor time_to_load.
tonyg: Are we creating more test binaries these days or trying to consolidate them? Can you look over the gyp change?
lgtm with tonyg's approval. https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... File chrome/browser/load_library_perf_test.cc (right): https://codereview.chromium.org/151283003/diff/710001/chrome/browser/load_lib... chrome/browser/load_library_perf_test.cc:18: base::FilePath library_path = output_dir.Append(library_name); On 2014/02/12 20:49:24, shadi1 wrote: > On 2014/02/12 20:25:06, Lei Zhang wrote: > > Just use AppendASCII() so MeasureTimeToLoadNativeLibraryByBaseName() don't > have > > to do all these utf16 conversions. > > I wanted |library_name| to be a FilePath in case the library is within other > folders in DIR_MODULE, example inside lib folder. or did I misunderstood you? Ok, that's fine. I thought it's just a simple name, in which case we can simplify a little.
Ping. tonyg@ can you PTAL?
I have a question about the merit of a load library benchmark. As you are probably aware, the load library implementation has a lot of flexibility on most platforms in terms of how aggressively it maps in pages, as well as behaviors about whether they will be sequential, random, and how much lookahead there is. We could easily improve this benchmark by making it more lazy, only to cause an even greater increase in end user latency due to increased page faults later on. In principle, I'd much rather have a Telemetry test that measures something users care about (such as time to first video frame) in a scenario where library loading is on the critical path such as browser startup or playing an encrypted video. If you disagree, I wont stand in the way of checking in this benchmark. I just want to make sure its results are appropriately caveated and would request that we also have something that measures the user experienced responsiveness scenario. > tonyg: Are we creating more test binaries these days or trying to consolidate them? The issue is that for every new binary, we have to make a master.cfg change and restart, which is manual work and disruptive. It would be nice to have one native_perftests target which runs all native perf tests (including the cc_perftests, media_perftests, this, and others). Then adding a new native perf test would be just like adding a new native functional test. However, we don't have such a target yet. I'd be fine if you want to add "just one more binary" or if you want to create the one-binary-to-rule-them-all first.
https://codereview.chromium.org/151283003/diff/780001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/151283003/diff/780001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2817: If we want to run this on the chromium.perf bots, we should add a dependency on this from chromium_builder_perf in all.gyp.
On Fri, Feb 14, 2014 at 10:11 AM, <tonyg@chromium.org> wrote: > I have a question about the merit of a load library benchmark. As you are > probably aware, the load library implementation has a lot of flexibility > on most > platforms in terms of how aggressively it maps in pages, as well as > behaviors > about whether they will be sequential, random, and how much lookahead > there is. > > We could easily improve this benchmark by making it more lazy, only to > cause an > even greater increase in end user latency due to increased page faults > later on. > In principle, I'd much rather have a Telemetry test that measures something > users care about (such as time to first video frame) in a scenario where > library > loading is on the critical path such as browser startup or playing an > encrypted > video. > The purpose of this CL was due to crbug.com/333028. A regression in loading libraries in sandbox affects time to start Chrome in general and not just time to get first frame. We do have plans to add time to play encrypted content for the long run but we actually want to also measure in specific time to load the libraries during boot up. > > If you disagree, I wont stand in the way of checking in this benchmark. I > just > want to make sure its results are appropriately caveated and would request > that > we also have something that measures the user experienced responsiveness > scenario. > > I agree, and as I mentioned, we do have plans to add user-experienced perf tests for encrypted media separately. > > tonyg: Are we creating more test binaries these days or trying to >> consolidate >> > them? > > The issue is that for every new binary, we have to make a master.cfg > change and > restart, which is manual work and disruptive. > > It would be nice to have one native_perftests target which runs all native > perf > tests (including the cc_perftests, media_perftests, this, and others). Then > adding a new native perf test would be just like adding a new native > functional > test. SGTM, but would rather have it out of scope of this CL :) > > However, we don't have such a target yet. I'd be fine if you want to add > "just > one more binary" or if you want to create the one-binary-to-rule-them-all > first. > > https://codereview.chromium.org/151283003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Feb 14, 2014 at 10:37 AM, Shadi Khalek <shadi@google.com> wrote: > On Fri, Feb 14, 2014 at 10:11 AM, <tonyg@chromium.org> wrote: > >> I have a question about the merit of a load library benchmark. As you are >> probably aware, the load library implementation has a lot of flexibility >> on most >> platforms in terms of how aggressively it maps in pages, as well as >> behaviors >> about whether they will be sequential, random, and how much lookahead >> there is. >> >> We could easily improve this benchmark by making it more lazy, only to >> cause an >> even greater increase in end user latency due to increased page faults >> later on. >> In principle, I'd much rather have a Telemetry test that measures >> something >> users care about (such as time to first video frame) in a scenario where >> library >> loading is on the critical path such as browser startup or playing an >> encrypted >> video. >> > > The purpose of this CL was due to crbug.com/333028. A regression in > loading libraries in sandbox affects time to start Chrome in general and > not just time to get first frame. We do have plans to add time to play > encrypted content for the long run but we actually want to also measure in > specific time to load the libraries during boot up. > To clarify, on Linux and Chrome OS, all libraries must be loaded before the zygote process sandboxes itself. On Mac and Windows, libraries are loaded before the renderer or plugin process sandboxes itself. Thus, I'm not sure lazy loading is an option. Are we missing something? This test is primarily to detect regressions. Testing specific libraries allows us to pinpoint the cause - in the above bug, the cause was not known for some time. I suppose we could/should have a zero-to-renderer test that would catch such issues AND be able to identify the exact cause because it would run against every CL. (Unfortunately, the existing testing for Chrome OS boot time does not appear to be run throughout the development lifecycle.) > >> If you disagree, I wont stand in the way of checking in this benchmark. I >> just >> want to make sure its results are appropriately caveated and would >> request that >> we also have something that measures the user experienced responsiveness >> scenario. >> >> I agree, and as I mentioned, we do have plans to add user-experienced > perf tests for encrypted media separately. > >> >> tonyg: Are we creating more test binaries these days or trying to >>> consolidate >>> >> them? >> >> The issue is that for every new binary, we have to make a master.cfg >> change and >> restart, which is manual work and disruptive. >> >> It would be nice to have one native_perftests target which runs all >> native perf >> tests (including the cc_perftests, media_perftests, this, and others). >> Then >> adding a new native perf test would be just like adding a new native >> functional >> test. > > > SGTM, but would rather have it out of scope of this CL :) > >> >> However, we don't have such a target yet. I'd be fine if you want to add >> "just >> one more binary" or if you want to create the one-binary-to-rule-them-all >> first. >> >> https://codereview.chromium.org/151283003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
tonyg@ PTAL
tonyg@ any comments?
pinging again tonyg....
pinging again tonyg....
lgtm
The CQ bit was checked by shadi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/151283003/940001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
The CQ bit was checked by shadi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/151283003/1010001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/151283003/1010001
Message was sent while issue was closed.
Change committed as 254088 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
