8350973: Add memory overhead counter to NMT #23921
Conversation
|
👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@gerard-ziemski The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
@gerard-ziemski this pull request can not be integrated into git checkout JDK-8350973
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
| @@ -282,7 +282,7 @@ void Arena::set_size_in_bytes(size_t size) { | |||
| if (_size_in_bytes != size) { | |||
| ssize_t delta = size - size_in_bytes(); | |||
| _size_in_bytes = size; | |||
| MemTracker::record_arena_size_change(delta, _mem_tag); | |||
| MemTracker::record_arena_size_change(delta, 0, _mem_tag); | |||
There was a problem hiding this comment.
Here the allocated param is passed as 0, which means that the allocated amount won't change whereas the requested amount will change. So the calculation of overhead in memReporter.cpp (size_t overhead = c->allocated() - c->requested();) results in a negative number and changed to unsigned and becomes a very large positive number. This may describe why the Aren overhead in NMT report is more than 100%. This also affects the total overhead reported at the beginning of the NMT report.
| @@ -101,6 +101,9 @@ void MemReporterBase::print_malloc(const MemoryCounter* c, MemTag mem_tag) const | |||
| out->print(" (peak=%zu%s #%zu)", | |||
| amount_in_current_scale(pk_amount), scale, pk_count); | |||
| } | |||
| size_t overhead = c->allocated() - c->requested(); | |||
There was a problem hiding this comment.
Maybe better to have an assert(c->allocated() >= c->requested(), "should be");.
| _malloc[chunk_idx].record_free(arena_size); | ||
| _all_mallocs.deallocate(arena_size); | ||
| _malloc[chunk_idx].record_free(arena_size, 0); | ||
| _all_mallocs.deallocate(arena_size, 0); |
There was a problem hiding this comment.
Here also the allocated param is passed as 0. This makes the accounting incorrect. The requested changes but allocated not.
| * The counters are updated atomically. | ||
| */ | ||
| class MemoryCounter { | ||
| private: | ||
| volatile size_t _count; | ||
| volatile size_t _size; | ||
| volatile size_t _requested; // how much bytes we asked for |
| } | ||
| } | ||
|
|
||
| inline size_t count() const { return Atomic::load(&_count); } | ||
| inline size_t size() const { return Atomic::load(&_size); } | ||
| inline size_t size() const { return Atomic::load(&_requested); } |
There was a problem hiding this comment.
Since there is no _size anymore, it is better to remove size() as well.
| } | ||
|
|
||
| inline void record_arena_size_change(ssize_t sz) { | ||
| _arena.resize(sz); | ||
| inline void record_arena_size_change(ssize_t requested, size_t allocated) { |
There was a problem hiding this comment.
allocated can also be negative since it is delta. So ssize_t for its type.
| inline size_t malloc_overhead() const { | ||
| return _all_mallocs.count() * MallocHeader::malloc_overhead(); | ||
| return _all_mallocs.allocated() - _all_mallocs.requested(); |
There was a problem hiding this comment.
Do we need an assert for negative amounts of the result?
| @@ -141,7 +141,7 @@ class MemBaseline { | |||
| size_t malloc_tracking_overhead() const { | |||
| assert(baseline_type() != Not_baselined, "Not yet baselined"); | |||
| MemBaseline* bl = const_cast<MemBaseline*>(this); | |||
| return bl->_malloc_memory_snapshot.malloc_overhead(); | |||
| return bl->_malloc_memory_snapshot.nmt_overhead(); | |||
There was a problem hiding this comment.
Better name for nmt_overhead() would be malloc_tracking_overhead() to be similar to its caller as here.
| const NativeCallStack& stack) { | ||
| assert(mem_base != nullptr, "caller should handle null"); | ||
| if (enabled()) { | ||
| return MallocTracker::record_malloc(mem_base, size, mem_tag, stack); | ||
| //fprintf(stderr, "record_malloc:%p:%zu\n", mem_base, requested); |
| #elif defined(__APPLE__) | ||
| #include <malloc/malloc.h> | ||
| static size_t raw_malloc_size(void* ptr) { ALLOW_C_FUNCTION(::malloc_size, return ::malloc_size(ptr);) } | ||
| //static size_t raw_malloc_size(void* ptr) { size_t allocated = _raw_malloc_size(ptr); fprintf(stderr, "%p:%zu\n", ptr, allocated); return allocated; } |
|
@gerard-ziemski This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
Progress
Error
Warnings
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23921/head:pull/23921$ git checkout pull/23921Update a local copy of the PR:
$ git checkout pull/23921$ git pull https://git.openjdk.org/jdk.git pull/23921/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23921View PR using the GUI difftool:
$ git pr show -t 23921Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23921.diff