Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(741)

Issue 2727693004: DEPS: Update leveldatabase from 1.18 to 1.20. (Closed)

Created:
3 years, 9 months ago by pwnall
Modified:
3 years, 9 months ago
Reviewers:
cmumford
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

DEPS: Update leveldatabase from 1.18 to 1.20. 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/2727693004 Cr-Commit-Position: refs/heads/master@{#454154} Committed: https://chromium.googlesource.com/chromium/src/+/c0182b5f45e7161cad9d586b75b66653575f46ce

Patch Set 1 #

Total comments: 4

Patch Set 2 : 1.20 commit hash. #

Patch Set 3 : Addressed feedback. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -4 lines) Patch
M DEPS View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/leveldatabase/BUILD.gn View 1 2 2 chunks +16 lines, -0 lines 0 comments Download
M third_party/leveldatabase/README.chromium View 1 chunk +1 line, -1 line 0 comments Download
M third_party/leveldatabase/port/port_chromium.h View 1 2 1 chunk +5 lines, -2 lines 1 comment Download

Messages

Total messages: 19 (12 generated)
cmumford
https://codereview.chromium.org/2727693004/diff/20001/third_party/leveldatabase/BUILD.gn File third_party/leveldatabase/BUILD.gn (right): https://codereview.chromium.org/2727693004/diff/20001/third_party/leveldatabase/BUILD.gn#newcode25 third_party/leveldatabase/BUILD.gn:25: defines += [ "LEVELDB_PLATFORM_POSIX_SSE=1" ] Can you remove the ...
3 years, 9 months ago (2017-03-02 00:01:45 UTC) #5
pwnall
Thank you very much for the feedback! PTAL? https://codereview.chromium.org/2727693004/diff/20001/third_party/leveldatabase/BUILD.gn File third_party/leveldatabase/BUILD.gn (right): https://codereview.chromium.org/2727693004/diff/20001/third_party/leveldatabase/BUILD.gn#newcode25 third_party/leveldatabase/BUILD.gn:25: defines ...
3 years, 9 months ago (2017-03-02 00:28:25 UTC) #6
cmumford
lgtm. I'd probably not use issue 533648. We only suspect that a faster leveldb would ...
3 years, 9 months ago (2017-03-02 00:36:02 UTC) #9
pwnall
On 2017/03/02 00:36:02, cmumford wrote: > lgtm. I'd probably not use issue 533648. We only ...
3 years, 9 months ago (2017-03-02 00:39:01 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2727693004/60001
3 years, 9 months ago (2017-03-02 02:18:19 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c0182b5f45e7161cad9d586b75b66653575f46ce
3 years, 9 months ago (2017-03-02 02:28:32 UTC) #18
James Cook
3 years, 9 months ago (2017-03-02 03:18:49 UTC) #19
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/2723373002/ by jamescook@chromium.org.

The reason for reverting is: Looks like this broke the Windows builders. Not
sure how it passed CQ:

https://build.chromium.org/p/chromium/builders/Win/builds/52433

FAILED: obj/third_party/leveldatabase/leveldb_cache_test/cache_test.obj 
ninja -t msvc -e environment.x86 -- C:\b\c\goma_client/gomacc.exe
"C:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/third_party/leveldatabase/leveldb_cache_test/cache_test.obj.rsp /c
../../third_party/leveldatabase/src/util/cache_test.cc
/Foobj/third_party/leveldatabase/leveldb_cache_test/cache_test.obj
/Fd"obj/third_party/leveldatabase/leveldb_cache_test_cc.pdb"
c:\b\c\b\win_archive\src\third_party\leveldatabase\src\util\cache_test.cc(167):
error C2220: warning treated as error - no 'object' file generated
c:\b\c\b\win_archive\src\third_party\leveldatabase\src\util\cache_test.cc(167):
warning C4018: '<': signed/unsigned mismatch
c:\b\c\b\win_archive\src\third_party\leveldatabase\src\util\cache_test.cc(171):
warning C4018: '<': signed/unsigned mismatch

.

Powered by Google App Engine
This is Rietveld 408576698