Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2275353002:
Removes TestVector from ct_log_verifier_unittest (Closed)
|
Created:
4 years, 3 months ago by Rob Percival Modified:
4 years, 3 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoves TestVector from ct_log_verifier_unittest
Its only purpose was to bundle together a hex string and the number of
bytes it represents. The number of bytes (length_bytes) is redundant
though, as it is easily calculated by strlen(str) / 2, and is not used
anyway. The one thing that appeared to use it was HexToBytes, but this
contained a bug that meant it both misinterpreted and ignored it.
This also optimizes GetEmptyTreeHash by only calculating it once, which
reduces total test duration by ~1 second.
BUG=631087
Committed: https://crrev.com/540ba6cbe242920544a1f43019e4f4a3c6f81bf8
Cr-Commit-Position: refs/heads/master@{#414552}
Patch Set 1 #Patch Set 2 : Removes comment referring to TestVector #
Total comments: 8
Patch Set 3 : Addresses Ryan Sleevi's comments #
Total comments: 3
Patch Set 4 : Addresses final comment #Patch Set 5 : Fixes "truncation of constant value" warning #Messages
Total messages: 21 (9 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...
robpercival@chromium.org changed reviewers: + rsleevi@chromium.org
PTAL - improvement on https://codereview.chromium.org/2183073002/
https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:98: std::string HexToBytes(const std::string& hex_data) { DESIGN: You're (almost) always passing string constants to this function, but forcing them to be copied into std::string()'s, which adds overhead. A StringPiece would avoid the copy, but because the API contract itself is a constant string with no embedded NULLs, is there any reason not to do std::string HexToBytes(const char* hex_data) https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:107: static const std::string empty_hash = HexToBytes(kSHA256EmptyTreeHash); STYLE: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables "Variables of class type with static storage duration are forbidden"
I think your comment about removing HexToBytes entirely (on https://codereview.chromium.org/2182533002) is a good idea. I'll put together a change following this that does so. https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:98: std::string HexToBytes(const std::string& hex_data) { On 2016/08/25 18:32:11, Ryan Sleevi (slow) wrote: > DESIGN: You're (almost) always passing string constants to this function, but > forcing them to be copied into std::string()'s, which adds overhead. > > A StringPiece would avoid the copy, but because the API contract itself is a > constant string with no embedded NULLs, is there any reason not to do > > std::string HexToBytes(const char* hex_data) base::HexStringToBytes takes a const std::string&, so the overhead has to be paid anyway. https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:107: static const std::string empty_hash = HexToBytes(kSHA256EmptyTreeHash); On 2016/08/25 18:32:11, Ryan Sleevi (slow) wrote: > STYLE: > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > "Variables of class type with static storage duration are forbidden" Fixed by changing kSHA256EmptyTreeHash to be the decoded bytes, and just returning that. It's less efficient but avoids the static std::string.
https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:98: std::string HexToBytes(const std::string& hex_data) { On 2016/08/25 19:35:13, Rob Percival wrote: > base::HexStringToBytes takes a const std::string&, so the overhead has to be > paid anyway. So then, is this a good API to be using? Should you just store the raw byte literals then, to avoid the convenience API that's adding measurable overhead to test execution? That is, \x00\x01\x02 etc https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:107: static const std::string empty_hash = HexToBytes(kSHA256EmptyTreeHash); On 2016/08/25 19:35:13, Rob Percival wrote: > Fixed by changing kSHA256EmptyTreeHash to be the decoded bytes, and just > returning that. It's less efficient but avoids the static std::string. DESIGN: Given that you're clearly concerned about performance, why not const char*? :)
https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:98: std::string HexToBytes(const std::string& hex_data) { On 2016/08/25 20:15:20, Ryan Sleevi (slow) wrote: > On 2016/08/25 19:35:13, Rob Percival wrote: > > base::HexStringToBytes takes a const std::string&, so the overhead has to be > > paid anyway. > > So then, is this a good API to be using? Should you just store the raw byte > literals then, to avoid the convenience API that's adding measurable overhead to > test execution? > > That is, > \x00\x01\x02 etc As I said in a separate comment, I'm in the process of making that change now (in a follow-up CL). https://codereview.chromium.org/2275353002/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:107: static const std::string empty_hash = HexToBytes(kSHA256EmptyTreeHash); On 2016/08/25 20:15:20, Ryan Sleevi (slow) wrote: > On 2016/08/25 19:35:13, Rob Percival wrote: > > Fixed by changing kSHA256EmptyTreeHash to be the decoded bytes, and just > > returning that. It's less efficient but avoids the static std::string. > > DESIGN: Given that you're clearly concerned about performance, why not const > char*? :) Passing around hashes as char* runs the risk that it'll get implicitly converted into a std::string at some point, and there lie subtle bugs when the hash contains a null byte. The only performance impact I observed was the one caused by executing HexToBytes for the same empty hash over and over. I'm far less concerned about the cost of string copies.
Once your (other) CL lands, I'll take a pass, because I think the constant use of hex is a real detriment in this code. Below is one suggestion to avoid an unnecessary header dependency, which itself doesn't give you the memory safety you're trying to achieve. https://codereview.chromium.org/2275353002/diff/40001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2275353002/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:16: #include "crypto/sha2.h" Unnecessary https://codereview.chromium.org/2275353002/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:54: const char* const kSHA256EmptyTreeHash = const char kSHA256EmptyTreeHash[32] = { 0xe3, 0xb0, 0xc4, ... }; https://codereview.chromium.org/2275353002/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_unittest.cc:109: return std::string(kSHA256EmptyTreeHash, crypto::kSHA256Length); return std::string(std::begin(kSHA256EmptyTreeHash), std::end(kSHA256EmptyTreeHash));
lgtm
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/2275353002/#ps60001 (title: "Addresses final 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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/2275353002/#ps80001 (title: "Fixes "truncation of constant value" warning")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Removes TestVector from ct_log_verifier_unittest Its only purpose was to bundle together a hex string and the number of bytes it represents. The number of bytes (length_bytes) is redundant though, as it is easily calculated by strlen(str) / 2, and is not used anyway. The one thing that appeared to use it was HexToBytes, but this contained a bug that meant it both misinterpreted and ignored it. This also optimizes GetEmptyTreeHash by only calculating it once, which reduces total test duration by ~1 second. BUG=631087 ========== to ========== Removes TestVector from ct_log_verifier_unittest Its only purpose was to bundle together a hex string and the number of bytes it represents. The number of bytes (length_bytes) is redundant though, as it is easily calculated by strlen(str) / 2, and is not used anyway. The one thing that appeared to use it was HexToBytes, but this contained a bug that meant it both misinterpreted and ignored it. This also optimizes GetEmptyTreeHash by only calculating it once, which reduces total test duration by ~1 second. BUG=631087 Committed: https://crrev.com/540ba6cbe242920544a1f43019e4f4a3c6f81bf8 Cr-Commit-Position: refs/heads/master@{#414552} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/540ba6cbe242920544a1f43019e4f4a3c6f81bf8 Cr-Commit-Position: refs/heads/master@{#414552} |
