Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2187043004:
Makes an expensive CTLogVerifier test into a value-parameterized test (Closed)
|
Created:
4 years, 4 months ago by Rob Percival Modified:
4 years, 4 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes an expensive CTLogVerifier test into a value-parameterized test
As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/,
this turns one test case with nested loops into a value-parameterized test.
This avoids having a single test case that runs for ~13s in exchange for
128 test cases which each take ~1-350ms each (proportional to the tree size). It reduces the chance of the test case
timing out, but increases the overall test duration by ~3s.
BUG=598406
Committed: https://crrev.com/844c3d233e4b9a47cb1d8a5b2f60811200a8310c
Cr-Commit-Position: refs/heads/master@{#408534}
Patch Set 1 #Patch Set 2 : Simplifies generation of test params #Patch Set 3 : Improves documentation and adds missing #includes #
Total comments: 2
Patch Set 4 : Parameterize tests only on tree size #Patch Set 5 : Adds SCOPED_TRACE to loop in test #
Total comments: 4
Patch Set 6 : Addresses Ryan's comments #
Total comments: 1
Patch Set 7 : Fixes compilation on 64-bit architectures #Messages
Total messages: 26 (14 generated)
The CQ bit was checked by robpercival@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 ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 8513 test cases which each take ~3ms. It reduces the chance of test case timing out, but increases the overall test duration by ~11s. BUG=598406 ========== to ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 8513 test cases which each take ~3ms. It reduces the chance of the test case timing out, but increases the overall test duration by ~11s. BUG=598406 ==========
Patchset #3 (id:40001) has been deleted
robpercival@chromium.org changed reviewers: + rsleevi@chromium.org
PTAL
Description was changed from ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 8513 test cases which each take ~3ms. It reduces the chance of the test case timing out, but increases the overall test duration by ~11s. BUG=598406 ========== to ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 8256 test cases which each take ~3ms. It reduces the chance of the test case timing out, but increases the overall test duration by ~11s. BUG=598406 ==========
Mostly LG, but a Q on a design decision here below https://codereview.chromium.org/2187043004/diff/60001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2187043004/diff/60001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:532: testing::ValuesIn(GenerateTestParams(128))); Doesn't this 128 magic value tightly couple with the 256 on line 517? Regarding the right level of sharding of the test and the # of tests vs performance overhead - did you look at moving the inner loop from 501-505 to be part of the test (that is, moving 520-526 to be inside a loop) That is, the GetParam() returns the tree_size to test, and the test itself tests all leaves within that tree_size. That should still considerably shorten the tests, but reduces the number of tests to just being 128 tests. INSTANTIATE_TEST_CASE_P(..., ..., testing::Range(1, 128))
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2187043004/diff/60001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2187043004/diff/60001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:532: testing::ValuesIn(GenerateTestParams(128))); On 2016/07/28 16:46:50, Ryan Sleevi (slow) wrote: > Doesn't this 128 magic value tightly couple with the 256 on line 517? Indeed it does - I've created a constant to link them. > Regarding the right level of sharding of the test and the # of tests vs > performance overhead - did you look at moving the inner loop from 501-505 to be > part of the test (that is, moving 520-526 to be inside a loop) > > That is, the GetParam() returns the tree_size to test, and the test itself tests > all leaves within that tree_size. That should still considerably shorten the > tests, but reduces the number of tests to just being 128 tests. > > INSTANTIATE_TEST_CASE_P(..., ..., testing::Range(1, 128)) Done. That strikes a better balance between individual test duration (each test case now takes ~250ms) and overall test duration (~18s).
LGTM with two nits, and a request you update the CL description to reflect the final state. https://codereview.chromium.org/2187043004/diff/120001/net/cert/ct_log_verifi... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2187043004/diff/120001/net/cert/ct_log_verifi... net/cert/ct_log_verifier_unittest.cc:505: std::string root2 = ReferenceMerkleTreeHash(data.data(), tree_size); DESIGN: I believe you can move this to the outer-loop like you used to (see 506 vs 508). That is, you can move this up to be between lines 501/502 and gain better performance. Might help to make const std::string root2 (or rename the variables based on the order of decl; that is, root1 becomes the tree_size root, root2 because the snapshot root) https://codereview.chromium.org/2187043004/diff/120001/net/cert/ct_log_verifi... net/cert/ct_log_verifier_unittest.cc:515: testing::Range(1ul, (kReferenceTreeSize / 2) + 1)); nit: 1u should be sufficient (C++11 will promote it up to uint64_t if it's unsigned int)
Description was changed from ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 8256 test cases which each take ~3ms. It reduces the chance of the test case timing out, but increases the overall test duration by ~11s. BUG=598406 ========== to ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 128 test cases which each take ~1-350ms each (proportional to the tree size). It reduces the chance of the test case timing out, but increases the overall test duration by ~3s. BUG=598406 ==========
https://codereview.chromium.org/2187043004/diff/120001/net/cert/ct_log_verifi... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2187043004/diff/120001/net/cert/ct_log_verifi... net/cert/ct_log_verifier_unittest.cc:505: std::string root2 = ReferenceMerkleTreeHash(data.data(), tree_size); On 2016/07/28 18:03:31, Ryan Sleevi (slow) wrote: > DESIGN: I believe you can move this to the outer-loop like you used to (see 506 > vs 508). That is, you can move this up to be between lines 501/502 and gain > better performance. Might help to make const std::string root2 > > (or rename the variables based on the order of decl; that is, root1 becomes the > tree_size root, root2 because the snapshot root) Done. https://codereview.chromium.org/2187043004/diff/120001/net/cert/ct_log_verifi... net/cert/ct_log_verifier_unittest.cc:515: testing::Range(1ul, (kReferenceTreeSize / 2) + 1)); On 2016/07/28 18:03:31, Ryan Sleevi (slow) wrote: > nit: 1u should be sufficient (C++11 will promote it up to uint64_t if it's > unsigned int) Results in compilation error: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned int' vs. 'unsigned long') internal::ParamGenerator<T> Range(T start, T end) {
Thanks. LGTM still.
The CQ bit was checked by robpercival@chromium.org
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: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2187043004/diff/140001/net/cert/ct_log_verifi... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2187043004/diff/140001/net/cert/ct_log_verifi... net/cert/ct_log_verifier_unittest.cc:518: testing::Range(1ul, (kReferenceTreeSize / 2) + 1)); UINT64_C(1) should resolve the compile issues :)
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2187043004/#ps160001 (title: "Fixes compilation on 64-bit architectures")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 128 test cases which each take ~1-350ms each (proportional to the tree size). It reduces the chance of the test case timing out, but increases the overall test duration by ~3s. BUG=598406 ========== to ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 128 test cases which each take ~1-350ms each (proportional to the tree size). It reduces the chance of the test case timing out, but increases the overall test duration by ~3s. BUG=598406 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 128 test cases which each take ~1-350ms each (proportional to the tree size). It reduces the chance of the test case timing out, but increases the overall test duration by ~3s. BUG=598406 ========== to ========== Makes an expensive CTLogVerifier test into a value-parameterized test As recommended by Ryan Sleevi in https://codereview.chromium.org/2178153003/, this turns one test case with nested loops into a value-parameterized test. This avoids having a single test case that runs for ~13s in exchange for 128 test cases which each take ~1-350ms each (proportional to the tree size). It reduces the chance of the test case timing out, but increases the overall test duration by ~3s. BUG=598406 Committed: https://crrev.com/844c3d233e4b9a47cb1d8a5b2f60811200a8310c Cr-Commit-Position: refs/heads/master@{#408534} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/844c3d233e4b9a47cb1d8a5b2f60811200a8310c Cr-Commit-Position: refs/heads/master@{#408534} |
