Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2730703002:
DEPS: Update leveldatabase from 1.18 to 1.20. (Closed)
DescriptionDEPS: Update leveldatabase from 1.18 to 1.20.
This is a re-land of http://crrev.com/2727693004
This speeds up CRC32C computation from ~900MB/s to ~4GB/s on computers
with modern Intel CPUs. Snappy decompression runs at around 4GB/s, so
CRC32C is a limiting factor when reading large blocks, which can happen
when we store large values.
BUG=
Review-Url: https://codereview.chromium.org/2730703002
Cr-Commit-Position: refs/heads/master@{#454393}
Committed: https://chromium.googlesource.com/chromium/src/+/cfb346f113303aea2124c7a7d61768664294e17a
Patch Set 1 : http://crrev.com/2727693004 #Patch Set 2 : Build tweaks to get leveldb tests compiled on CQ. #Patch Set 3 : Bugfix + refactoring for build trickery. #Patch Set 4 : Suppress Visual Studio 2012 warning in cache_test.cc. #
Total comments: 2
Patch Set 5 : Remove build hacks. #
Total comments: 2
Patch Set 6 : Addressed cmumford feedback. #Patch Set 7 : Fix long line. #
Messages
Total messages: 35 (22 generated)
The CQ bit was checked by pwnall@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...
pwnall@chromium.org changed reviewers: + cmumford@chromium.org, jsbell@chromium.org, thakis@chromium.org
thakis@chromium.org: Please review changes in BUILD.gn jsbell@chromium.org: Please review changes in storage/BUILD.gn cmumford@chromium.org: Please review changes compared to http://crrev.com/2727693004 Patch Set 1 is the CL that got reverted. The changes in Patch Sets 2 and 3 make it so that leveldb tests are compiled on CQ bots. Proof that the goal is met: the CQ builds [1, 2] in Patch Set 3 have the same compilation error as the build bot [3] that got the original CL reverted. Patch Set 4 suppresses a Visual Studio warning to make the build errors go away. This is also done elsewhere in leveldb's BUILD.gn file. Also, FWIW, the warnings do not show when I build on my Windows box with Visual Studio 2015 Update 3. [1] https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_com... [2] https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_com... [3] https://build.chromium.org/p/chromium/builders/Win/builds/52433/steps/compile...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis@chromium.org changed reviewers: + dpranke@google.com
BUILD.gn lgtm, but the part marked with "dpranke: ^" below looks a bit fishy to me – Dirk, can you comment on if that's ok or if there's something better? I guess the "something better" would be to add the leveldb tests to some of the bots in testing/buildbot, which could easily part of this CL. https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.gn File storage/browser/BUILD.gn (right): https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.g... storage/browser/BUILD.gn:241: # Trick for getting leveldb tests compiled on the CQ. http://crbug.com/697788 dpranke: ^
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Yes, this is "fishy", i.e., no lgtm :) https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.gn File storage/browser/BUILD.gn (right): https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.g... storage/browser/BUILD.gn:241: # Trick for getting leveldb tests compiled on the CQ. http://crbug.com/697788 On 2017/03/02 14:11:27, Nico wrote: > dpranke: ^ Don't do this, it's making the build incorrect by claiming that there are dependencies that aren't really there. In addition, you're trying to subvert policies that were put in place intentionally. If you want to ensure that particular targets are built as part of the CQ, there is an approved way to do that: add them to the `additional_compile_targets` entries for a given builder in the //testing/buildbot/*.json files.
The CQ bit was checked by pwnall@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...
On 2017/03/02 16:35:00, Dirk Pranke wrote: > Yes, this is "fishy", i.e., no lgtm :) > > https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.gn > File storage/browser/BUILD.gn (right): > > https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.g... > storage/browser/BUILD.gn:241: # Trick for getting leveldb tests compiled on the > CQ. http://crbug.com/697788 > On 2017/03/02 14:11:27, Nico wrote: > > dpranke: ^ > > Don't do this, it's making the build incorrect by claiming that there are > dependencies that aren't really there. > In addition, you're trying to subvert policies that were put in place > intentionally. > > If you want to ensure that particular targets are built as part of the CQ, there > is an approved way to do that: add them to the `additional_compile_targets` > entries for a given builder in the //testing/buildbot/*.json files. Thank you for your guidance! I removed the storage/BUILD.gn change. It has already accomplished its purpose for this CL -- The bot results for Patch Sets #3 and #4 give me a reasonable degree of confidence that cache_test.cc is the only file that needs warning suppression right now. I'll follow up about changes to //testing/buildbot/*.json and introduce them in another CL, if it makes sense. Based on the comments above, I think that the BUILD.gn change is still acceptable, as it's just a refactoring. There should be no actual changes in the build dependency graph, and now it's less likely that we'd add a test in third_party/leveldatabase/BUILD.gn that wouldn't end up in gn_all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/02 18:51:11, pwnall wrote: > On 2017/03/02 16:35:00, Dirk Pranke wrote: > > Yes, this is "fishy", i.e., no lgtm :) > > > > https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.gn > > File storage/browser/BUILD.gn (right): > > > > > https://codereview.chromium.org/2730703002/diff/60001/storage/browser/BUILD.g... > > storage/browser/BUILD.gn:241: # Trick for getting leveldb tests compiled on > the > > CQ. http://crbug.com/697788 > > On 2017/03/02 14:11:27, Nico wrote: > > > dpranke: ^ > > > > Don't do this, it's making the build incorrect by claiming that there are > > dependencies that aren't really there. > > In addition, you're trying to subvert policies that were put in place > > intentionally. > > > > If you want to ensure that particular targets are built as part of the CQ, > there > > is an approved way to do that: add them to the `additional_compile_targets` > > entries for a given builder in the //testing/buildbot/*.json files. > > Thank you for your guidance! > > I removed the storage/BUILD.gn change. It has already accomplished its purpose > for this CL -- The bot results for Patch Sets #3 and #4 give me a reasonable > degree of confidence that cache_test.cc is the only file that needs warning > suppression right now. I'll follow up about changes to //testing/buildbot/*.json > and introduce them in another CL, if it makes sense. > > Based on the comments above, I think that the BUILD.gn change is still > acceptable, as it's just a refactoring. There should be no actual changes in the > build dependency graph, and now it's less likely that we'd add a test in > third_party/leveldatabase/BUILD.gn that wouldn't end up in gn_all. dpranke: It just occurred to me that you might have meant to block the CL. Can you PTAL and see if what I'm proposing above LGTY?
You are correct, I did mean to block the CL :). The new version is fine, thanks for fixing. lgtm.
https://codereview.chromium.org/2730703002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/BUILD.gn (right): https://codereview.chromium.org/2730703002/diff/80001/third_party/leveldataba... third_party/leveldatabase/BUILD.gn:209: # util\cache_test.cc(167): warning C4018: '<': signed/unsigned mismatch Let's just fix the test. we'll be using leveldb v1.20+, but I think that's better than disabling this warning. If you don't want to miss M58 today then at least add a TODO.
The CQ bit was checked by pwnall@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...
cmumford: Thanks for the thoughtful feedback! PTAL? https://codereview.chromium.org/2730703002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/BUILD.gn (right): https://codereview.chromium.org/2730703002/diff/80001/third_party/leveldataba... third_party/leveldatabase/BUILD.gn:209: # util\cache_test.cc(167): warning C4018: '<': signed/unsigned mismatch On 2017/03/02 20:29:50, cmumford wrote: > Let's just fix the test. we'll be using leveldb v1.20+, but I think that's > better than disabling this warning. > > If you don't want to miss M58 today then at least add a TODO. Done. This flag is also used for most of leveldb's source code (see line 134 above), so I suspect the fixes will be a bit involved.
The CQ bit was checked by pwnall@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...
pwnall@chromium.org changed reviewers: - dpranke@google.com, jsbell@chromium.org
Moving jsbell@ to cc, because this CL doesn't touch storage/BUILD.gn anymore.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2730703002/#ps120001 (title: "Fix long line.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you very much for the speedy review, everyone! I know the timing isn't great, and I'm super-grateful.
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488491551996630,
"parent_rev": "9f81209c0afcb72f576f4794c28c11520b35d7bb", "commit_rev":
"cfb346f113303aea2124c7a7d61768664294e17a"}
Message was sent while issue was closed.
Description was changed from ========== DEPS: Update leveldatabase from 1.18 to 1.20. This is a re-land of http://crrev.com/2727693004 This speeds up CRC32C computation from ~900MB/s to ~4GB/s on computers with modern Intel CPUs. Snappy decompression runs at around 4GB/s, so CRC32C is a limiting factor when reading large blocks, which can happen when we store large values. BUG= ========== to ========== DEPS: Update leveldatabase from 1.18 to 1.20. This is a re-land of http://crrev.com/2727693004 This speeds up CRC32C computation from ~900MB/s to ~4GB/s on computers with modern Intel CPUs. Snappy decompression runs at around 4GB/s, so CRC32C is a limiting factor when reading large blocks, which can happen when we store large values. BUG= Review-Url: https://codereview.chromium.org/2730703002 Cr-Commit-Position: refs/heads/master@{#454393} Committed: https://chromium.googlesource.com/chromium/src/+/cfb346f113303aea2124c7a7d617... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/cfb346f113303aea2124c7a7d617... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
