libroach: Fix memory leak in row_counter #42529
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt and @petermattis)
c-deps/libroach/row_counter.cc, line 94 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
Yeah... You're right... I'm totally unfamiliar with this code base.
Anyways, Fixed the comparison plus added row counter destructor to avoid leaking the last "prev_key" memory
Looks like instead of DBString we can use ToString method and do string comparison. Not sure whether we need to worry about nulls inside rockdb::Slice, that's why I kept it as it was before, just fixed comparison as you indicated. I simply don't know if std::string is appropriate here. Let me know.
There was a problem hiding this comment.
@miretskiy Thanks for tackling this. Something to be aware of is that DBString needs to be a plain C struct so that it can be used for Go. I think the author of the code here thought there was another reason for using DBString, but there really is no reason to use it unless we're returning data to Go (which isn't taking place here). For pure C++ code like this, we should be using std::string.
Reviewable status:
c-deps/libroach/row_counter.cc, line 94 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
Looks like instead of DBString we can use ToString method and do string comparison. Not sure whether we need to worry about nulls inside rockdb::Slice, that's why I kept it as it was before, just fixed comparison as you indicated. I simply don't know if std::string is appropriate here. Let me know.
std::string is definitely appropriate. Much more appropriate than using DBString which is intended only for passing data from this C++ code back to Go. We do have to worry about NUL bytes in the strings, but both std::string and rocksdb::Slice handle them properly.
My suggestion, change the type of prev_key to std::string. The line of code below then becomes:
prev_key.assign(decoded_key.data(), decoded_key.size())
The comparison above becomes decoded_key == prev_key.
This approach has the nice side-effect of reducing memory allocations as well as prev_key.assign will reuse the underlying storage when possible.
Sounds great. Done. |
Free previous key memory when counting row keys. Release note (bug fix): Do not leak memory when counting rows during backup.
|
bors r+ |
42529: libroach: Fix memory leak in row_counter r=miretskiy a=miretskiy Free previous key memory when counting row keys. Release note (bug fix): Do not leak memory when counting rows during backup. Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Build succeeded |
|
This should be backported, right? 19.1? 19.2? both?
…On Sun, Nov 17, 2019 at 8:21 PM craig[bot] ***@***.***> wrote:
Merged #42529 <#42529> into
master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#42529?email_source=notifications&email_token=ANA4FVCBDTC4F5C7OBEEMU3QUHU3JA5CNFSM4JOLLY72YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOU47BLMY#event-2805863859>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVHB5RIGGDBQCS5VMCDQUHU3JANCNFSM4JOLLY7Q>
.
|
|
Definitely should be backported to 19.2 -- was introduced in 19.2. |
There was a problem hiding this comment.
Reviewable status:
c-deps/libroach/row_counter.h, line 33 at r2 (raw file):
int GetRowPrefixLength(rocksdb::Slice* key); std::string prev_key; rocksdb::Slice prev;
Is prev unused?
|
It does appear to be unused :(
Do we not use compiler warnings for these things (-WUnused, etc)?
…On Mon, Nov 18, 2019 at 9:19 AM Peter Mattis ***@***.***> wrote:
***@***.**** commented on this pull request.
*Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/42529>*
status: [image:
] complete! 0 of 0 LGTMs obtained (waiting on @dt
<https://github.com/dt> and @miretskiy <https://github.com/miretskiy>)
------------------------------
*c-deps/libroach/row_counter.h, line 33 at r2
<https://reviewable.io/reviews/cockroachdb/cockroach/42529#-LtysBtk7eiSeV0xFITS:-LtysBtl-E-j1ZQtTdja:b-kkj89l>
(raw file
<https://github.com/cockroachdb/cockroach/blob/52c1f3a537100007ae1a58937746f86190a26b69/c-deps/libroach/row_counter.h#L33>):*
int GetRowPrefixLength(rocksdb::Slice* key);
std::string prev_key;
rocksdb::Slice prev;
Is prev unused?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#42529?email_source=notifications&email_token=ANA4FVAUNQ5TQ5Y2CAWG5ZLQUKP57A5CNFSM4JOLLY72YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCL45DQI#pullrequestreview-318362049>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVG3RYFCPRXF7XQC2C3QUKP57ANCNFSM4JOLLY7Q>
.
|
|
What's the C++ tool that we should've used to catch this? |
Our C++ build process is a bit unloved and it is mixed in with the Cmake files provided by RocksDB and our other C++ dependencies. We should be using compiler warnings.
Catch the memory leak? I recall there being a leak detector built in to the C++ unit test framework used inside Google (gunit). My memory on how this worked is hazy. It might have depended on tcmalloc as well. @miretskiy ? |
|
I've been playing with this today...
Google used clang address sanitizer, which also has super light weight
leak detector (-fsanitize=leak). Alas, clang on Mac does not support it.
I got things partially working with fsanitize=address, but I'm running into
a bit of trouble because of cgo (my guess is that not everything gets
compiled with sanitizer and that causes issues)...
…On Mon, Nov 18, 2019, 4:43 PM Peter Mattis ***@***.***> wrote:
Do we not use compiler warnings for these things (-WUnused, etc)?
Our C++ build process is a bit unloved and it is mixed in with the Cmake
files provided by RocksDB and our other C++ dependencies. We *should* be
using compiler warnings.
What's the C++ tool that we should've used to catch this?
Catch the memory leak? I recall there being a leak detector built in to
the C++ unit test framework used inside Google (gunit). My memory on how
this worked is hazy. It might have depended on tcmalloc as well.
@miretskiy <https://github.com/miretskiy> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#42529?email_source=notifications&email_token=ANA4FVAC5LZUQUKHVBQJDUTQUMEBBA5CNFSM4JOLLY72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEMAO5Y#issuecomment-555222903>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVGJVSQHEYNSCD76JVLQUMEBBANCNFSM4JOLLY7Q>
.
|
|
We use |
|
From a quick glance it looks like jemalloc is not entirely an online tool
in a sense that it requires running external program after program shutdown
to analyze data.
-fsanitize=XXX is online: tests do not pass if there is a leak, etc...
The downside is that you actually have to exercise the code in question to
detect leaks/races/etc...
Which is why I was looking at CGO layer because it doesn't appear that the
cpp code has sufficient coverage
…On Mon, Nov 18, 2019 at 5:56 PM Ben Darnell ***@***.***> wrote:
We use jemalloc for c++ memory allocation, which has good profiling
tools. I haven't tried the leak checker but we should see if it works here:
https://github.com/jemalloc/jemalloc/wiki/Use-Case:-Leak-Checking
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#42529?email_source=notifications&email_token=ANA4FVHGGZCUNXWP4DYZ6TLQUMMRLA5CNFSM4JOLLY72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEMGYUA#issuecomment-555248720>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVE7K4PO6AUJOJD5UUTQUMMRLANCNFSM4JOLLY7Q>
.
|
Free previous key memory when counting row keys.
Release note (bug fix): Do not leak memory when counting rows during backup.