You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/03/12 20:37:42 UTC
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6359
to review the following change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
TIL that the pointer returned by std::make_shared().get() or
*std::make_shared() isn't the base of the underlying heap memory allocation,
because the shared_ptr control block _precedes_ the object pointer.
This means that kudu_malloc_usable_size(this) is totally unsafe if 'this'
was allocated via std::make_shared(). With glibc's malloc, it caused a
crash. Thankfully tcmalloc is more forgiving: it finds its allocations'
bookkeeping information by converting addresses into their underlying pages,
so a crash would only occur if there was a page boundary between the
shared_ptr control block and the object's pointer address (I don't even know
if this is possible).
I found this bug by accident when I ran an in-progress unit test with
--block_manager=file. This speaks to our lack of FBM coverage. For now, I've
added some coverage but we really should be running at least one end-to-end
test on all block managers.
Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
3 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6359/1
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
PS1, Line 438: LOG(INFO)
> It is useful: this is the only call to ReadableBlock::memory_footprint() in
makes sense.
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> _Sp_counted_base isn't portable: there's no class with the same name in lib
could you just add sizeof(shared_ptr<Descriptor<RandomAccessFile>>) ? Anecdotally I tried it on my mac and is does indeed return 16bytes
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> But sizeof(shared_ptr<Descriptor<RandomAccessFile>>) isn't really correct.
right, pardon my "doh" moment.
If there is no platform independent way to get the size of the control block maybe we should consider switching to scoped_refptr?
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6359
to look at the new patch set (#3).
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
TIL that the pointer returned by std::make_shared().get() or
*std::make_shared() isn't the base of the underlying heap memory allocation,
because the shared_ptr control block _precedes_ the object pointer.
This means that kudu_malloc_usable_size(this) is totally unsafe if 'this'
was allocated via std::make_shared(). With glibc's malloc, it caused a
crash. Thankfully tcmalloc is more forgiving: it finds its allocations'
bookkeeping information by converting addresses into their underlying pages,
so a crash would only occur if there was a page boundary between the
shared_ptr control block and the object's pointer address (I don't even know
if this is possible).
I found this bug by accident when I ran an in-progress unit test with
--block_manager=file. This speaks to our lack of FBM coverage. For now, I've
added some coverage but we really should be running at least one end-to-end
test on all block managers; I filed KUDU-1932 to track that.
Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
3 files changed, 38 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6359/3
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> right, pardon my "doh" moment.
We could, but that's a more invasive change, as shared_ptr is used consistently for all (or almost all) RandomAccessFiles.
FWIW, I don't think underaccounting the memory usage is that bad; the vast majority of heap allocations in Kudu aren't individually accounted for today.
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6359
to look at the new patch set (#2).
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
TIL that the pointer returned by std::make_shared().get() or
*std::make_shared() isn't the base of the underlying heap memory allocation,
because the shared_ptr control block _precedes_ the object pointer.
This means that kudu_malloc_usable_size(this) is totally unsafe if 'this'
was allocated via std::make_shared(). With glibc's malloc, it caused a
crash. Thankfully tcmalloc is more forgiving: it finds its allocations'
bookkeeping information by converting addresses into their underlying pages,
so a crash would only occur if there was a page boundary between the
shared_ptr control block and the object's pointer address (I don't even know
if this is possible).
I found this bug by accident when I ran an in-progress unit test with
--block_manager=file. This speaks to our lack of FBM coverage. For now, I've
added some coverage but we really should be running at least one end-to-end
test on all block managers.
Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
3 files changed, 35 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6359/2
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
TIL that the pointer returned by std::make_shared().get() or
*std::make_shared() isn't the base of the underlying heap memory allocation,
because the shared_ptr control block _precedes_ the object pointer.
This means that kudu_malloc_usable_size(this) is totally unsafe if 'this'
was allocated via std::make_shared(). With glibc's malloc, it caused a
crash. Thankfully tcmalloc is more forgiving: it finds its allocations'
bookkeeping information by converting addresses into their underlying pages,
so a crash would only occur if there was a page boundary between the
shared_ptr control block and the object's pointer address (I don't even know
if this is possible).
I found this bug by accident when I ran an in-progress unit test with
--block_manager=file. This speaks to our lack of FBM coverage. For now, I've
added some coverage but we really should be running at least one end-to-end
test on all block managers; I filed KUDU-1932 to track that.
Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Reviewed-on: http://gerrit.cloudera.org:8080/6359
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
3 files changed, 38 insertions(+), 1 deletion(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> could you just add sizeof(shared_ptr<Descriptor<RandomAccessFile>>) ? Anecd
But sizeof(shared_ptr<Descriptor<RandomAccessFile>>) isn't really correct. Yes, the shared_ptr is 16 bytes, but that's 8 bytes of "pointer to control block" and 8 bytes of "pointer to object". What we want is the size of the control block itself, which is 8 bytes of weak ref count and 8 bytes of strong ref count.
I guess what I'm saying is, if you want me to add 16 to the total, I can do that, but it's arguably more correct than adding sizeof() of an unrelated object (the shared_ptr itself).
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6359/1//COMMIT_MSG
Commit Message:
PS1, Line 22: For now, I've
: added some coverage but we really should be running at least one end-to-end
: test on all block managers.
> can you create a jira to make some bm tests parameterized so that we run th
The block manager tests (block_manager-test and block_manager-stress-test) _are_ parameterized on both; the issue is that neither test suites ever invoke ReadableBlock::memory_footprint(), because it's really something that is useful to test at this low level (short of just calling it to make sure it doesn't crash).
Tests that do invoke memory_footprint() are even higher level (i.e. cfile-test, tablet-test, any cluster test) and never run against the FBM.
I filed KUDU-1932.
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
PS1, Line 438: LOG(INFO)
> is this useful without further context? consider moving to VLOG
It is useful: this is the only call to ReadableBlock::memory_footprint() in all of our block manager parameterized tests. That's why I added it, and that's why it shouldn't be VLOG(), where it won't run by default.
I'll add a comment explaining this so no one will remove it by accident.
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
PS1, Line 291: class RandomAccessFileCacheTest : public FileCacheTest<RandomAccessFile> {
: };
> why not make this a typed test like the others? still covers what you want
That won't work; memory_footprint() is only a method on RandomAccessFile, not RWFile.
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> would making the descriptor inherit from enable_shared_from this help? alte
_Sp_counted_base isn't portable: there's no class with the same name in libc++. I'm not even sure if libc++'s shared_ptr control block is the same size; I took a quick glance at the code and its implementation is quite different. I don't think it's worth the effort to conditionally do something for libc++ and something else for libstdc++.
I first experimented with enable_shared_from_this() and saw some interesting results. Within memory_footprint(), the address was still offset by the size of the shared_ptr control block. But instead of crashing, malloc_usable_size() returned a garbage value (-16). The code I was using looked like this:
size_t memory_footprint() const override {
- return kudu_malloc_usable_size(this) +
+ return kudu_malloc_usable_size(shared_from_this().get()) +
once_.memory_footprint_excluding_this() +
base_.filename().capacity();
}
It makes sense that enable_shared_from_this doesn't help. Yes, it lets you construct shared_ptrs using 'this', but those shared_ptrs are stack objects, and calling .get() yields the same object pointer as if enable_shared_from_this wasn't in use, one where the base of the allocation is actually 16 bytes prior.
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> If you look at some other accounted objects, you'll see that they either ex
What I meant was that there could easily be situations where a class is both with and without a shared_ptr, and if it doesn't inherit from RefCounted there's no way to account for the different uses. Particularly I was saying that that I'm ok with not accounting for the control block given the current uses of this method but that we should leave a note here (as an add on to the notes you already have) so that if we even add a memory_footprint() to FileChache we don't forget to add the shared_ptr control block size there as, at least in the fbm, that might become meaningful.
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
Line 354: // - sizeof(*this): 72 bytes.
why not add in the sizeof _sp_counted_base?
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> We could, but that's a more invasive change, as shared_ptr is used consiste
that's a fair argument. also technically that measurement doesn't belong here (how's this class to know that it's always allocated with a shared_ptr). We should make note of it if we ever use it to measure the memory footprint of the file cache though.
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
> that's a fair argument. also technically that measurement doesn't belong he
If you look at some other accounted objects, you'll see that they either expose a single memory_footprint() method, or two memory_footprint_{including,excluding}_this() methods. The sole memory_footprint() is the same memory_footprint_including_this(), and implies that the class is always heap allocated and is behind a shared_ptr, unique_ptr, or raw pointer.
That's the convention we've been following, loosely inspired by how Firefox accounts for its memory consumption. It works, but it's brittle, as the methods need to change if:
1. The class allocation scheme changes, or
2. The class gains a new heap allocated subobject.
Anyway, I didn't understand part of your last comment: what exactly did you want me to note?
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory footprint()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1931: fix SIGSEGV in file cache RandomAccessFile::memory_footprint()
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6359/1//COMMIT_MSG
Commit Message:
PS1, Line 22: For now, I've
: added some coverage but we really should be running at least one end-to-end
: test on all block managers.
can you create a jira to make some bm tests parameterized so that we run them with both block managers?
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
PS1, Line 438: LOG(INFO)
is this useful without further context? consider moving to VLOG
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
PS1, Line 291: class RandomAccessFileCacheTest : public FileCacheTest<RandomAccessFile> {
: };
why not make this a typed test like the others? still covers what you want to cover, right?
http://gerrit.cloudera.org:8080/#/c/6359/1/src/kudu/util/file_cache.cc
File src/kudu/util/file_cache.cc:
PS1, Line 350: // Some anecdotal memory measurements taken inside gdb:
: // - glibc 2.23 malloc_usable_size() on make_shared<FileType>: 88 bytes.
: // - tcmalloc malloc_usable_size() on make_shared<FileType>: 96 bytes.
: // - sizeof(std::shared_ptr::_Sp_counted_base): 16 bytes.
: // - sizeof(*this): 72 bytes.
would making the descriptor inherit from enable_shared_from this help? alternatively could you add the size of the shared_ptr control block?
--
To view, visit http://gerrit.cloudera.org:8080/6359
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e57c06dd35252d03729c10ff64fe43d79288513
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes