[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: JDK-8171119: Low-Overhead Heap Profiling
From: "White, Derek" <Derek.White () cavium ! com>
Date: 2018-03-30 23:24:09
Message-ID: BL0PR07MB3890D4312B84EF7BD596B43484A10 () BL0PR07MB3890 ! namprd07 ! prod ! outlook ! com
[Download RAW message or body]
[Attachment #2 (text/plain)]
Hi Jc,
I've been having trouble getting your patch to apply correctly. I may have based it \
on the wrong version.
In any case, I think there's a missing update to macroAssembler_aarch64.cpp, in \
MacroAssembler::tlab_allocate(), where "JavaThread::tlab_end_offset()" should become \
"JavaThread::tlab_current_end_offset()".
This should correspond to the other port's changes in templateTable_<cpu>.cpp files.
Thanks!
- Derek
From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces@openjdk.java.net] On \
Behalf Of JC Beyler
Sent: Wednesday, March 28, 2018 11:43 AM
To: Erik Österlund <erik.osterlund@oracle.com>
Cc: serviceability-dev@openjdk.java.net; hotspot-compiler-dev \
<hotspot-compiler-dev@openjdk.java.net>
Subject: Re: JDK-8171119: Low-Overhead Heap Profiling
Hi all,
I've been working on deflaking the tests mostly and the wording in the JVMTI spec.
Here is the two incremental webrevs:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.5_6/
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.06_07/
Here is the total webrev:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.07/
Here are the notes of this change:
- Currently the tests pass 100 times in a row, I am working on checking if they \
pass 1000 times in a row.
- The default sampling rate is set to 512k, this is what we use internally and \
having a default means that to enable the sampling with the default, the user only \
has to do a enable event/disable event via JVMTI (instead of enable + set sample \
rate).
- I deprecated the code that was handling the fast path tlab refill if it happened \
since this is now deprecated
- Though I saw that Graal is still using it so I have to see what needs to be \
done there exactly
Finally, using the Dacapo benchmark suite, I noted a 1% overhead for when the event \
system is turned on and the callback to the native agent is just empty. I got a 3% \
overhead with a 512k sampling rate with the code I put in the native side of my \
tests.
Thanks and comments are appreciated,
Jc
On Mon, Mar 19, 2018 at 2:06 PM JC Beyler \
<jcbeyler@google.com<mailto:jcbeyler@google.com>> wrote: Hi all,
The incremental webrev update is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event4_5/
The full webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event5/
Major change here is:
- I've removed the heapMonitoring.cpp code in favor of just having the sampling \
events as per Serguei's request; I still have to do some overhead measurements but \
the tests prove the concept can work
- Most of the tlab code is unchanged, the only major part is that now things \
get sent off to event collectors when used and enabled.
- Added the interpreter collectors to handle interpreter execution
- Updated the name from SetTlabHeapSampling to SetHeapSampling to be more generic
- Added a mutex for the thread sampling so that we can initialize an internal \
static array safely
- Ported the tests from the old system to this new one
I've also updated the JEP and CSR to reflect these changes:
https://bugs.openjdk.java.net/browse/JDK-8194905
https://bugs.openjdk.java.net/browse/JDK-8171119
In order to make this have some forward progress, I've removed the heap sampling code \
entirely and now rely entirely on the event sampling system. The tests reflect this \
by using a simplified implementation of what an agent could do: \
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event5/raw_files/new/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitor.c
(Search for anything mentioning event_storage).
I have not taken the time to port the whole code we had originally in heapMonitoring \
to this. I hesitate only because that code was in C++, I'd have to port it to C and \
this is for tests so perhaps what I have now is good enough?
As far as testing goes, I've ported all the relevant tests and then added a few:
- Turning the system on/off
- Testing using various GCs
- Testing using the interpreter
- Testing the sampling rate
- Testing with objects and arrays
- Testing with various threads
Finally, as overhead goes, I have the numbers of the system off vs a clean build and \
I have 0% overhead, which is what we'd want. This was using the Dacapo benchmarks. I \
am now preparing to run a version with the events on using dacapo and will report \
back here.
Any comments are welcome :)
Jc
On Thu, Mar 8, 2018 at 4:00 PM JC Beyler \
<jcbeyler@google.com<mailto:jcbeyler@google.com>> wrote: Hi all,
I apologize for the delay but I wanted to add an event system and that took a bit \
longer than expected and I also reworked the code to take into account the \
deprecation of FastTLABRefill.
This update has four parts:
A) I moved the implementation from Thread to ThreadHeapSampler inside of Thread. \
Would you prefer it as a pointer inside of Thread or like this works for you? Second \
question would be would you rather have an association outside of Thread altogether \
that tries to remember when threads are live and then we would have something like: \
ThreadHeapSampler::get_sampling_size(this_thread);
I worry about the overhead of this but perhaps it is not too too bad?
B) I also have been working on the Allocation event system that sends out a \
notification at each sampled event. This will be practical when wanting to do \
something at the allocation point. I'm also looking at if the whole heapMonitoring \
code could not reside in the agent code and not in the JDK. I'm not convinced but I'm \
talking to Serguei about it to see/assess :)
- Also added two tests for the new event subsystem
C) Removed the slow_path fields inside the TLAB code since now FastTLABRefill is \
deprecated
D) Updated the JVMTI documentation and specification for the methods.
So the incremental webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.09_10/
and the full webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.10
I believe I have updated the various JIRA issues that track this :)
Thanks for your input,
Jc
On Wed, Feb 14, 2018 at 10:34 PM, JC Beyler \
<jcbeyler@google.com<mailto:jcbeyler@google.com>> wrote: Hi Erik,
I inlined my answers, which the last one seems to answer Robbin's concerns about the \
same thing (adding things to Thread).
On Wed, Feb 14, 2018 at 2:51 AM, Erik Österlund \
<erik.osterlund@oracle.com<mailto:erik.osterlund@oracle.com>> wrote: Hi JC,
Comments are inlined below.
On 2018-02-13 06:18, JC Beyler wrote:
Hi Erik,
Thanks for your answers, I've now inlined my own answers/comments.
I've done a new webrev here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.08/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.08/>
The incremental is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.07_08/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.07_08/>
Note to all:
- I've been integrating changes from Erin/Serguei/David comments so this webrev \
incremental is a bit an answer to all comments in one. I apologize for that :)
On Mon, Feb 12, 2018 at 6:05 AM, Erik Österlund \
<erik.osterlund@oracle.com<mailto:erik.osterlund@oracle.com>> wrote: Hi JC,
Sorry for the delayed reply.
Inlined answers:
On 2018-02-06 00:04, JC Beyler wrote:
Hi Erik,
(Renaming this to be folded into the newly renamed thread :))
First off, thanks a lot for reviewing the webrev! I appreciate it!
I updated the webrev to:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05a/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.05a/>
And the incremental one is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.04_05a/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.04_05a/>
It contains:
- The change for since from 9 to 11 for the jvmti.xml
- The use of the OrderAccess for initialized
- Clearing the oop
I also have inlined my answers to your comments. The biggest question
will come from the multiple *_end variables. A bit of the logic there
is due to handling the slow path refill vs fast path refill and
checking that the rug was not pulled underneath the slowpath. I
believe that a previous comment was that TlabFastRefill was going to
be deprecated.
If this is true, we could revert this code a bit and just do a : if
TlabFastRefill is enabled, disable this. And then deprecate that when
TlabFastRefill is deprecated.
This might simplify this webrev and I can work on a follow-up that
either: removes TlabFastRefill if Robbin does not have the time to do
it or add the support to the assembly side to handle this correctly.
What do you think?
I support removing TlabFastRefill, but I think it is good to not depend on that \
happening first.
I'm slowly pushing on the FastTLABRefill \
(https://bugs.openjdk.java.net/browse/JDK-8194084), I agree on keeping both separate \
for now though so that we can think of both differently
Now, below, inlined are my answers:
On Fri, Feb 2, 2018 at 8:44 AM, Erik Österlund
<erik.osterlund@oracle.com<mailto:erik.osterlund@oracle.com>> wrote:
Hi JC,
Hope I am reviewing the right version of your work. Here goes...
src/hotspot/share/gc/shared/collectedHeap.inline.hpp:
159 AllocTracer::send_allocation_outside_tlab(klass, result, size *
HeapWordSize, THREAD);
160
161 THREAD->tlab().handle_sample(THREAD, result, size);
162 return result;
163 }
Should not call tlab()->X without checking if (UseTLAB) IMO.
Done!
More about this later.
src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp:
So first of all, there seems to quite a few ends. There is an "end", a "hard
end", a "slow path end", and an "actual end". Moreover, it seems like the
"hard end" is actually further away than the "actual end". So the "hard end"
seems like more of a "really definitely actual end" or something. I don't
know about you, but I think it looks kind of messy. In particular, I don't
feel like the name "actual end" reflects what it represents, especially when
there is another end that is behind the "actual end".
413 HeapWord* ThreadLocalAllocBuffer::hard_end() {
414 // Did a fast TLAB refill occur?
415 if (_slow_path_end != _end) {
416 // Fix up the actual end to be now the end of this TLAB.
417 _slow_path_end = _end;
418 _actual_end = _end;
419 }
420
421 return _actual_end + alignment_reserve();
422 }
I really do not like making getters unexpectedly have these kind of side
effects. It is not expected that when you ask for the "hard end", you
implicitly update the "slow path end" and "actual end" to new values.
As I said, a lot of this is due to the FastTlabRefill. If I make this
not supporting FastTlabRefill, this goes away. The reason the system
needs to update itself at the get is that you only know at that get if
things have shifted underneath the tlab slow path. I am not sure of
really better names (naming is hard!), perhaps we could do these
names:
- current_tlab_end // Either the allocated tlab end or a sampling point
- last_allocation_address // The end of the tlab allocation
- last_slowpath_allocated_end // In case a fast refill occurred the
end might have changed, this is to remember slow vs fast past refills
the hard_end method can be renamed to something like:
tlab_end_pointer() // The end of the lab including a bit of
alignment reserved bytes
Those names sound better to me. Could you please provide a mapping from the old names \
to the new names so I understand which one is which please?
This is my current guess of what you are proposing:
end -> current_tlab_end
actual_end -> last_allocation_address
slow_path_end -> last_slowpath_allocated_end
hard_end -> tlab_end_pointer
Yes that is correct, that was what I was proposing.
I would prefer this naming:
end -> slow_path_end // the end for taking a slow path; either due to sampling or \
refilling actual_end -> allocation_end // the end for allocations
slow_path_end -> last_slow_path_end // last address for slow_path_end (as opposed to \
allocation_end) hard_end -> reserved_end // the end of the reserved space of the TLAB
About setting things in the getter... that still seems like a very unpleasant thing \
to me. It would be better to inspect the call hierarchy and explicitly update the \
ends where they need updating, and assert in the getter that they are in sync, rather \
than implicitly setting various ends as a surprising side effect in a getter. It \
looks like the call hierarchy is very small. With my new naming convention, \
reserved_end() would presumably return _allocation_end + alignment_reserve(), and \
have an assert checking that _allocation_end == _last_slow_path_allocation_end, \
complaining that this invariant must hold, and that a caller to this function, such \
as make_parsable(), must first explicitly synchronize the ends as required, to honor \
that invariant.
I've renamed the variables to how you preferred it except for the _end one. I did:
current_end
last_allocation_address
tlab_end_ptr
The reason is that the architecture dependent code use the thread.hpp API and it \
already has tlab included into the name so it becomes tlab_current_end (which is \
better that tlab_current_tlab_end in my opinion).
I also moved the update into a separate method with a TODO that says to remove it \
when FastTLABRefill is deprecated
This looks a lot better now. Thanks.
Note that the following comment now needs updating accordingly in \
threadLocalAllocBuffer.hpp:
41 // Heap sampling is performed via the end/actual_end fields.
42 // actual_end contains the real end of the tlab allocation,
43 // whereas end can be set to an arbitrary spot in the tlab to
44 // trip the return and sample the allocation.
45 // slow_path_end is used to track if a fast tlab refill occured
46 // between slowpath calls.
There might be other comments too, I have not looked in detail.
This was the only spot that still had an actual_end, I fixed it now. I'll do a sweep \
to double check other comments.
Not sure it's better but before updating the webrev, I wanted to try
to get input/consensus :)
(Note hard_end was always further off than end).
src/hotspot/share/prims/jvmti.xml:
10357 <capabilityfield id="can_sample_heap" since="9">
10358 <description>
10359 Can sample the heap.
10360 If this capability is enabled then the heap sampling methods
can be called.
10361 </description>
10362 </capabilityfield>
Looks like this capability should not be "since 9" if it gets integrated
now.
Updated now to 11, crossing my fingers :)
src/hotspot/share/runtime/heapMonitoring.cpp:
448 if (is_alive->do_object_b(value)) {
449 // Update the oop to point to the new object if it is still
alive.
450 f->do_oop(&(trace.obj));
451
452 // Copy the old trace, if it is still live.
453 _allocated_traces->at_put(curr_pos++, trace);
454
455 // Store the live trace in a cache, to be served up on /heapz.
456 _traces_on_last_full_gc->append(trace);
457
458 count++;
459 } else {
460 // If the old trace is no longer live, add it to the list of
461 // recently collected garbage.
462 store_garbage_trace(trace);
463 }
In the case where the oop was not live, I would like it to be explicitly
cleared.
Done I think how you wanted it. Let me know because I'm not familiar
with the RootAccess API. I'm unclear if I'm doing this right or not so
reviews of these parts are highly appreciated. Robbin had talked of
perhaps later pushing this all into a OopStorage, should I do this now
do you think? Or can that wait a second webrev later down the road?
I think using handles can and should be done later. You can use the Access API now.
I noticed that you are missing an #include "oops/access.inline.hpp" in your \
heapMonitoring.cpp file.
The missing header is there for me so I don't know, I made sure it is present in the \
latest webrev. Sorry about that.
+ Did I clear it the way you wanted me to or were you thinking of
something else?
That is precisely how I wanted it to be cleared. Thanks.
+ Final question here, seems like if I were to want to not do the
f->do_oop directly on the trace.obj, I'd need to do something like:
f->do_oop(&value);
...
trace->store_oop(value);
to update the oop internally. Is that right/is that one of the
advantages of going to the Oopstorage sooner than later?
I think you really want to do the do_oop on the root directly. Is there a particular \
reason why you would not want to do that? Otherwise, yes - the benefit with using the \
handle approach is that you do not need to call do_oop explicitly in your code.
There is no reason except that now we have a load_oop and a get_oop_addr, I was not \
sure what you would think of that.
That's fine.
Also I see a lot of concurrent-looking use of the following field:
267 volatile bool _initialized;
Please note that the "volatile" qualifier does not help with reordering
here. Reordering between volatile and non-volatile fields is completely free
for both compiler and hardware, except for windows with MSVC, where volatile
semantics is defined to use acquire/release semantics, and the hardware is
TSO. But for the general case, I would expect this field to be stored with
OrderAccess::release_store and loaded with OrderAccess::load_acquire.
Otherwise it is not thread safe.
Because everything is behind a mutex, I wasn't really worried about
this. I have a test that has multiple threads trying to hit this
corner case and it passes.
However, to be paranoid, I updated it to using the OrderAccess API
now, thanks! Let me know what you think there too!
If it is indeed always supposed to be read and written under a mutex, then I would \
strongly prefer to have it accessed as a normal non-volatile member, and have an \
assertion that given lock is held or we are in a safepoint, as we do in many other \
places. Something like this:
assert(HeapMonitorStorage_lock->owned_by_self() || \
(SafepointSynchronize::is_at_safepoint() && Thread::current()->is_VM_thread()), "this \
should not be accessed concurrently");
It would be confusing to people reading the code if there are uses of OrderAccess \
that are actually always protected under a mutex.
Thank you for the exact example to be put in the code! I put it around each \
access/assignment of the _initialized method and found one case where yes you can \
touch it and not have the lock. It actually is "ok" because you don't act on the \
storage until later and only when you really want to modify the storage (see the \
object_alloc_do_sample method which calls the add_trace method).
But, because of this, I'm going to put the OrderAccess here, I'll do some performance \
numbers later and if there are issues, I might add a "unsafe" read and a "safe" one \
to make it explicit to the reader. But I don't think it will come to that.
Okay. This double return in heapMonitoring.cpp looks wrong:
283 bool initialized() {
284 return OrderAccess::load_acquire(&_initialized) != 0;
285 return _initialized;
286 }
Since you said object_alloc_do_sample() is the only place where you do not hold the \
mutex while reading initialized(), I had a closer look at that. It looks like in its \
current shape, the lack of a mutex may lead to a memory leak. In particular, it first \
checks if (initialized()). Let's assume this is now true. It then allocates a bunch \
of stuff, and checks if the number of frames were over 0. If they were, it calls \
StackTraceStorage::storage()->add_trace() seemingly hoping that after grabbing the \
lock in there, initialized() will still return true. But it could now return false \
and skip doing anything, in which case the allocated stuff will never be freed.
I fixed this now by making add_trace return a boolean and checking for that. It will \
be in the next webrev. Thanks, the truth is that in our implementation the system is \
always on or off, so this never really occurs :). In this version though, that is not \
true and it's important to handle so thanks again!
So the analysis seems to be that _initialized is only used outside of the mutex in \
once instance, where it is used to perform double-checked locking, that actually \
causes a memory leak.
I am not proposing how to fix that, just raising the issue. If you still want to \
perform this double-checked locking somehow, then the use of acquire/release still \
seems odd. Because the memory ordering restrictions of it never comes into play in \
this particular case. If it ever did, then the use of destroy_stuff(); \
release_store(_initialized, 0) would be broken anyway as that would imply that \
whatever concurrent reader there ever was would after reading _initialized with \
load_acquire() could *never* read the data that is concurrently destroyed anyway. I \
would be biased to think that RawAccess<MO_RELAXED>::load/store looks like a more \
appropriate solution, given that the memory leak issue is resolved. I do not know how \
painful it would be to not perform this double-checked locking.
So I agree with this entirely. I looked also a bit more and the difference and code \
really stems from our internal version. In this version however, there are actually a \
lot of things going on that I did not go entirely through in my head but this comment \
made me ponder a bit more on it.
Since every object_alloc_do_sample is protected by a check to \
HeapMonitoring::enabled(), there is only a small chance that the call is happening \
when things have been disabled. So there is no real need to do a first check on the \
initialized, it is a rare occurence that a call happens to object_alloc_do_sample and \
the initialized of the storage returns false.
(By the way, even if you did call object_alloc_do_sample without looking at \
HeapMonitoring::enabled(), that would be ok too. You would gather the stacktrace and \
get nowhere at the add_trace call, which would return false; so though not optimal \
performance wise, nothing would break).
Furthermore, the add_trace is really the moment of no return and we have the mutex \
lock and then the initialized check. So, in the end, I did two things: I removed that \
first check and then I removed the OrderAccess for the storage initialized. I think \
now I have a better grasp and understanding why it was done in our code and why it is \
not needed here. Thanks for pointing it out :). This now still passes my JTREG tests, \
especially the threaded one.
As a kind of meta comment, I wonder if it would make sense to add sampling
for non-TLAB allocations. Seems like if someone is rapidly allocating a
whole bunch of 1 MB objects that never fit in a TLAB, I might still be
interested in seeing that in my traces, and not get surprised that the
allocation rate is very high yet not showing up in any profiles.
That is handled by the handle_sample where you wanted me to put a
UseTlab because you hit that case if the allocation is too big.
I see. It was not obvious to me that non-TLAB sampling is done in the TLAB class. \
That seems like an abstraction crime. What I wanted in my previous comment was that \
we do not call into the TLAB when we are not using TLABs. If there is sampling logic \
in the TLAB that is used for something else than TLABs, then it seems like that logic \
simply does not belong inside of the TLAB. It should be moved out of the TLAB, and \
instead have the TLAB call this common abstraction that makes sense.
So in the incremental version:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.07_08/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.07_08/>, \
this is still a "crime". The reason is that the system has to have the \
bytes_until_sample on a per-thread level and it made "sense" to have it with the TLAB \
implementation. Also, I was not sure how people felt about adding something to the \
thread instance instead.
Do you think it fits better at the Thread level? I can see how difficult it is to \
make it happen there and add some logic there. Let me know what you think.
We have an unfortunate situation where everyone that has some fields that are thread \
local tend to dump them right into Thread, making the size and complexity of Thread \
grow as it becomes tightly coupled with various unrelated subsystems. It would be \
desirable to have a separate class for this instead that encapsulates the sampling \
logic. That class could possibly reside in Thread though as a value object of Thread.
I imagined that would be the case but was not sure. I will look at the example that \
Robbin is talking about (ThreadSMR) and will see how to refactor my code to use that.
Thanks again for your help,
Jc
Hope I have answered your questions and that my feedback makes sense to you.
You have and thank you for them, I think we are getting to a cleaner implementation \
and things are getting better and more readable :)
Yes it is getting better.
Thanks,
/Erik
Thanks for your help!
Jc
Thanks,
/Erik
I double checked by changing the test
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05a/raw_files/new/test/hotspot/jtr \
eg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.jav \
a<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.05a/raw_files/new/test/hotspot \
/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.java>
to use a smaller Tlab (2048) and made the object bigger and it goes
through that and passes.
Thanks again for your review and I look forward to your pointers for
the questions I now have raised!
Jc
Thanks,
/Erik
On 2018-01-26 06:45, JC Beyler wrote:
Thanks Robbin for the reviews :)
The new full webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.03/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.03/>
The incremental webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02_03/<http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.02_03/>
I inlined my answers:
On Thu, Jan 25, 2018 at 1:15 AM, Robbin Ehn \
<robbin.ehn@oracle.com<mailto:robbin.ehn@oracle.com>> wrote: Hi JC, great to see \
another revision!
####
heapMonitoring.cpp
StackTraceData should not contain the oop for 'safety' reasons.
When StackTraceData is moved from _allocated_traces:
L452 store_garbage_trace(trace);
it contains a dead oop.
_allocated_traces could instead be a tupel of oop and StackTraceData thus
dead oops are not kept.
Done I used inheritance to make the copier work regardless but the
idea is the same.
You should use the new Access API for loading the oop, something like
this:
RootAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::load(...)
I don't think you need to use Access API for clearing the oop, but it
would
look nicer. And you shouldn't probably be using:
Universe::heap()->is_in_reserved(value)
I am unfamiliar with this but I think I did do it like you wanted me
to (all tests pass so that's a start). I'm not sure how to clear the
oop exactly, is there somewhere that does that, which I can use to do
the same?
I removed the is_in_reserved, this came from our internal version, I
don't know why it was there but my tests work without so I removed it
> )
The lock:
L424 MutexLocker mu(HeapMonitorStorage_lock);
Is not needed as far as I can see.
weak_oops_do is called in a safepoint, no TLAB allocation can happen and
JVMTI thread can't access these data-structures. Is there something more
to
this lock that I'm missing?
Since a thread can call the JVMTI getLiveTraces (or any of the other
ones), it can get to the point of trying to copying the
_allocated_traces. I imagine it is possible that this is happening
during a GC or that it can be started and a GC happens afterwards.
Therefore, it seems to me that you want this protected, no?
####
You have 6 files without any changes in them (any more):
g1CollectedHeap.cpp
psMarkSweep.cpp
psParallelCompact.cpp
genCollectedHeap.cpp
referenceProcessor.cpp
thread.hpp
Done.
####
I have not looked closely, but is it possible to hide heap sampling in
AllocTracer ? (with some minor changes to the AllocTracer API)
I am imagining that you are saying to move the code that does the
sampling code (change the tlab end, do the call to HeapMonitoring,
etc.) into the AllocTracer code itself? I think that is right and I'll
look if that is possible and prepare a webrev to show what would be
needed to make that happen.
####
Minor nit, when declaring pointer there is a little mix of having the
pointer adjacent by type name and data name. (Most hotspot code is by
type
name)
E.g.
heapMonitoring.cpp:711 jvmtiStackTrace *trace = ....
heapMonitoring.cpp:733 Method* m = vfst.method();
(not just this file)
Done!
####
HeapMonitorThreadOnOffTest.java:77
I would make g_tmp volatile, otherwise the assignment in loop may
theoretical be skipped.
Also done!
Thanks again!
Jc
[Attachment #3 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"> <head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new
{mso-style-name:m_-452193571787093578m_6512714288813904128m_68639937780844273m_8872727166074378132m_-513143501369679108new;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Hi Jc,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I've been having trouble getting your patch to apply correctly. \
I may have based it on the wrong version.<o:p></o:p></p> <p \
class="MsoNormal"><o:p> </o:p></p> <p class="MsoNormal">In any case, I think \
there's a missing update to macroAssembler_aarch64.cpp, in \
MacroAssembler::tlab_allocate(), where "JavaThread::tlab_end_offset()" should become \
"JavaThread::tlab_current_end_offset()".<o:p></o:p></p> <p \
class="MsoNormal"><o:p> </o:p></p> <p class="MsoNormal">This should correspond \
to the other port's changes in templateTable_<cpu>.cpp files.<o:p></o:p></p> <p \
class="MsoNormal"><o:p> </o:p></p> <p class="MsoNormal">Thanks!<br>
- Derek<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> hotspot-compiler-dev \
[mailto:hotspot-compiler-dev-bounces@openjdk.java.net] <b>On Behalf Of </b>JC \
Beyler<br> <b>Sent:</b> Wednesday, March 28, 2018 11:43 AM<br>
<b>To:</b> Erik Österlund <erik.osterlund@oracle.com><br>
<b>Cc:</b> serviceability-dev@openjdk.java.net; hotspot-compiler-dev \
<hotspot-compiler-dev@openjdk.java.net><br> <b>Subject:</b> Re: JDK-8171119: \
Low-Overhead Heap Profiling<o:p></o:p></p> </div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">Hi all,<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I've been working on deflaking the tests mostly and the wording \
in the JVMTI spec.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Here is the two incremental webrevs:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.5_6/">http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.5_6/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.06_07/">http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.06_07/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Here is the total webrev:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.07/">http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.07/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Here are the notes of this change:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - Currently the tests pass 100 times in a row, I am \
working on checking if they pass 1000 times in a row.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - The default sampling rate is set to 512k, this is what \
we use internally and having a default means that to enable the sampling with the \
default, the user only has to do a enable event/disable event via JVMTI (instead of \
enable + set sample rate).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - I deprecated the code that was handling the fast path \
tlab refill if it happened since this is now deprecated<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Though I saw that Graal is still using it \
so I have to see what needs to be done there exactly<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Finally, using the Dacapo benchmark suite, I noted a 1% overhead \
for when the event system is turned on and the callback to the native agent is just \
empty. I got a 3% overhead with a 512k sampling rate with the code I put in the \
native side of my tests.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thanks and comments are appreciated,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Mar 19, 2018 at 2:06 PM JC Beyler <<a \
href="mailto:jcbeyler@google.com">jcbeyler@google.com</a>> wrote:<o:p></o:p></p> \
</div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in \
0in 6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal">Hi all,<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The incremental webrev update is here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event4_5/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event4_5/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The full webrev is here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event5/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event5/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Major change here is:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - I've removed the heapMonitoring.cpp code in favor of \
just having the sampling events as per Serguei's request; I still have to do some \
overhead measurements but the tests prove the concept can work<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Most of the tlab code is unchanged, \
the only major part is that now things get sent off to event collectors when used and \
enabled.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Added the interpreter collectors to handle interpreter \
execution<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Updated the name from SetTlabHeapSampling to \
SetHeapSampling to be more generic<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Added a mutex for the thread sampling so that we can \
initialize an internal static array safely<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Ported the tests from the old system to this new \
one<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I've also updated the JEP and CSR to reflect these \
changes:<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> <a href="https://bugs.openjdk.java.net/browse/JDK-8194905" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8194905</a><o:p></o:p></p> \
</div> <div>
<p class="MsoNormal"> <a href="https://bugs.openjdk.java.net/browse/JDK-8171119" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8171119</a><o:p></o:p></p> \
</div> <div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">In order to make this have some forward progress, I've removed \
the heap sampling code entirely and now rely entirely on the event sampling system. \
The tests reflect this by using a simplified implementation of what an agent could \
do:<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event5/raw_files/new/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitor.c" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event5/raw_files/new \
/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitor.c</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">(Search for anything mentioning event_storage).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I have not taken the time to port the whole code we had \
originally in heapMonitoring to this. I hesitate only because that code was in \
C++, I'd have to port it to C and this is for tests so perhaps what I have \
now is good enough?<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">As far as testing goes, I've ported all the relevant tests and \
then added a few:<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Turning the system on/off<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - Testing using various GCs<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - Testing using the interpreter<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - Testing the sampling rate<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - Testing with objects and arrays<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - Testing with various threads<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Finally, as overhead goes, I have the numbers of the system off \
vs a clean build and I have 0% overhead, which is what we'd want. This was using the \
Dacapo benchmarks. I am now preparing to run a version with the events on using \
dacapo and will report back here.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Any comments are welcome :)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Thu, Mar 8, 2018 at 4:00 PM JC Beyler <<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>> \
wrote:<o:p></o:p></p> </div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal">Hi all,<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I apologize for the delay but I wanted to add an event system \
and that took a bit longer than expected and I also reworked the code to take into \
account the deprecation of FastTLABRefill.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">This update has four parts:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">A) I moved the implementation from Thread to ThreadHeapSampler \
inside of Thread. Would you prefer it as a pointer inside of Thread or like this \
works for you? Second question would be would you rather have an association outside \
of Thread altogether that tries to remember when threads are live and then we would \
have something like:<o:p></o:p></p> </div>
<div>
<p class="MsoNormal">ThreadHeapSampler::get_sampling_size(this_thread);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I worry about the overhead of this but perhaps it is not too too \
bad?<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">B) I also have been working on the Allocation event system that \
sends out a notification at each sampled event. This will be practical when wanting \
to do something at the allocation point. I'm also looking at if the whole \
heapMonitoring code could not reside in the agent code and not in the JDK. I'm not \
convinced but I'm talking to Serguei about it to see/assess :)<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> - Also added two tests for the new event \
subsystem<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">C) Removed the slow_path fields inside the TLAB code since now \
FastTLABRefill is deprecated<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">D) Updated the JVMTI documentation and specification for the \
methods.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">So the incremental webrev is here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.09_10/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.09_10/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">and the full webrev is here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a href="http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.10" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.10</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I believe I have updated the various JIRA issues that track this \
:)<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thanks for your input,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Wed, Feb 14, 2018 at 10:34 PM, JC Beyler <<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>> \
wrote:<o:p></o:p></p> <blockquote style="border:none;border-left:solid #CCCCCC \
1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal">Hi Erik,<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I inlined my answers, which the last one seems to answer \
Robbin's concerns about the same thing (adding things to Thread).<o:p></o:p></p> \
</div> <div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<div>
<p class="MsoNormal">On Wed, Feb 14, 2018 at 2:51 AM, Erik Österlund <<a \
href="mailto:erik.osterlund@oracle.com" \
target="_blank">erik.osterlund@oracle.com</a>> wrote:<o:p></o:p></p> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal">Hi JC,<br>
<br>
Comments are inlined below.<o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On 2018-02-13 06:18, JC Beyler wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal">Hi Erik, <o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thanks for your answers, I've now inlined my own \
answers/comments.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I've done a new webrev here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.08/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.08/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The incremental is here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.07_08/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.07_08/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Note to all:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> - I've been integrating changes from Erin/Serguei/David \
comments so this webrev incremental is a bit an answer to all comments in one. I \
apologize for that :)<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Mon, Feb 12, 2018 at 6:05 AM, Erik Österlund <<a \
href="mailto:erik.osterlund@oracle.com" \
target="_blank">erik.osterlund@oracle.com</a>> wrote:<o:p></o:p></p> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Hi JC,<br>
<br>
Sorry for the delayed reply.<br>
<br>
Inlined answers: <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal"><br>
<br>
On 2018-02-06 00:04, JC Beyler wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Hi Erik,<br>
<br>
(Renaming this to be folded into the newly renamed thread :))<br>
<br>
First off, thanks a lot for reviewing the webrev! I appreciate it!<br>
<br>
I updated the webrev to:<br>
<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.05a/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05a/</a><br> <br>
And the incremental one is here:<br>
<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.04_05a/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.04_05a/</a><br> \
<br> It contains:<br>
- The change for since from 9 to 11 for the jvmti.xml<br>
- The use of the OrderAccess for initialized<br>
- Clearing the oop<br>
<br>
I also have inlined my answers to your comments. The biggest question<br>
will come from the multiple *_end variables. A bit of the logic there<br>
is due to handling the slow path refill vs fast path refill and<br>
checking that the rug was not pulled underneath the slowpath. I<br>
believe that a previous comment was that TlabFastRefill was going to<br>
be deprecated.<br>
<br>
If this is true, we could revert this code a bit and just do a : if<br>
TlabFastRefill is enabled, disable this. And then deprecate that when<br>
TlabFastRefill is deprecated.<br>
<br>
This might simplify this webrev and I can work on a follow-up that<br>
either: removes TlabFastRefill if Robbin does not have the time to do<br>
it or add the support to the assembly side to handle this correctly.<br>
What do you think?<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt">I support removing TlabFastRefill, \
but I think it is good to not depend on that happening first.<o:p></o:p></p> \
</blockquote> <div>
<div>
<p class="MsoNormal"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222;background:white"><br>
I'm slowly pushing on the FastTLABRefill (<a href="https://bugs.openjdk.java.net" \
target="_blank">https://bugs.openjdk.java.net</a>/browse/JDK-8194084), I agree on \
keeping both separate for now though so that we can think of both \
differently</span><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p> </o:p></span></p>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Now, below, inlined \
are my answers:<br> <br>
On Fri, Feb 2, 2018 at 8:44 AM, Erik Österlund<br>
<<a href="mailto:erik.osterlund@oracle.com" \
target="_blank">erik.osterlund@oracle.com</a>> wrote:<o:p></o:p></p> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">Hi JC,<br> <br>
Hope I am reviewing the right version of your work. Here goes...<br>
<br>
src/hotspot/share/gc/shared/collectedHeap.inline.hpp:<br>
<br>
159 AllocTracer::send_allocation_outside_tlab(klass, \
result, size *<br> HeapWordSize, THREAD);<br>
160<br>
161 THREAD->tlab().handle_sample(THREAD, result, \
size);<br> 162 return result;<br>
163 }<br>
<br>
Should not call tlab()->X without checking if (UseTLAB) IMO.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal">Done!<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><br>
More about this later. <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal"><o:p> </o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp:<br>
<br>
So first of all, there seems to quite a few ends. There is an "end", a \
"hard<br> end", a "slow path end", and an "actual end". \
Moreover, it seems like the<br> "hard end" is actually further away than \
the "actual end". So the "hard end"<br> seems like more of a \
"really definitely actual end" or something. I don't<br> know about you, \
but I think it looks kind of messy. In particular, I don't<br> feel like the name \
"actual end" reflects what it represents, especially when<br> there is \
another end that is behind the "actual end".<br> <br>
413 HeapWord* ThreadLocalAllocBuffer::hard_end() {<br>
414 // Did a fast TLAB refill occur?<br>
415 if (_slow_path_end != _end) {<br>
416 // Fix up the actual end to be now the end of this \
TLAB.<br> 417 _slow_path_end = _end;<br>
418 _actual_end = _end;<br>
419 }<br>
420<br>
421 return _actual_end + alignment_reserve();<br>
422 }<br>
<br>
I really do not like making getters unexpectedly have these kind of side<br>
effects. It is not expected that when you ask for the "hard end", you<br>
implicitly update the "slow path end" and "actual end" to new \
values.<o:p></o:p></p> </blockquote>
<p class="MsoNormal">As I said, a lot of this is due to the FastTlabRefill. If I make \
this<br> not supporting FastTlabRefill, this goes away. The reason the system<br>
needs to update itself at the get is that you only know at that get if<br>
things have shifted underneath the tlab slow path. I am not sure of<br>
really better names (naming is hard!), perhaps we could do these<br>
names:<br>
<br>
- current_tlab_end // Either the allocated tlab end or a \
sampling point<br>
- last_allocation_address // The end of the tlab allocation<br>
- last_slowpath_allocated_end // In case a fast refill occurred the<br>
end might have changed, this is to remember slow vs fast past refills<br>
<br>
the hard_end method can be renamed to something like:<br>
tlab_end_pointer() // The end of the lab including a bit \
of<br> alignment reserved bytes<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt">Those names sound better to me. \
Could you please provide a mapping from the old names to the new names so I \
understand which one is which please?<br> <br>
This is my current guess of what you are proposing:<br>
<br>
end -> current_tlab_end<br>
actual_end -> last_allocation_address<br>
slow_path_end -> last_slowpath_allocated_end<br>
hard_end -> tlab_end_pointer<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Yes that is correct, that was what I was \
proposing.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">I would prefer this \
naming:<br> <br>
end -> slow_path_end // the end for taking a slow path; either due to sampling or \
refilling<br> actual_end -> allocation_end // the end for allocations<br>
slow_path_end -> last_slow_path_end // last address for slow_path_end (as opposed \
to allocation_end)<br> hard_end -> reserved_end // the end of the reserved space \
of the TLAB<br> <br>
About setting things in the getter... that still seems like a very unpleasant thing \
to me. It would be better to inspect the call hierarchy and explicitly update the \
ends where they need updating, and assert in the getter that they are in sync, rather \
than implicitly setting various ends as a surprising side effect in a getter. It \
looks like the call hierarchy is very small. With my new naming convention, \
reserved_end() would presumably return _allocation_end + alignment_reserve(), and \
have an assert checking that _allocation_end == _last_slow_path_allocation_end, \
complaining that this invariant must hold, and that a caller to this function, such \
as make_parsable(), must first explicitly synchronize the ends as required, to honor \
that invariant. <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><br> \
I've renamed the variables to how you preferred it except for the _end one. I \
did:<o:p></o:p></span></p> </div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222">current_end<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222;background:white">last_allocation_address</span><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222;background:white">tlab_end_ptr</span><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222;background:white">The \
reason is that the architecture dependent code use the thread.hpp API and it already \
has tlab included into the name so it becomes tlab_current_end (which is better that \
tlab_current_tlab_end in my opinion).</span><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222">I \
also moved the update into a separate method with a TODO that says to remove it when \
FastTLABRefill is deprecated<o:p></o:p></span></p> </div>
</div>
</div>
</div>
</div>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt">This looks a lot better now. \
Thanks.<br> <br>
Note that the following comment now needs updating accordingly in \
threadLocalAllocBuffer.hpp:<o:p></o:p></p> <pre><span \
class="m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new"> \
41 // Heap sampling \
is performed via the end/actual_end fields.</span><o:p></o:p></pre> <pre><span \
class="m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new"> \
42 // actual_end \
contains the real end of the tlab allocation,</span><o:p></o:p></pre> <pre><span \
class="m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new"> \
43 // whereas end \
can be set to an arbitrary spot in the tlab to</span><o:p></o:p></pre> <pre><span \
class="m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new"> \
44 // trip the \
return and sample the allocation.</span><o:p></o:p></pre> <pre><span \
class="m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new"> \
45 // slow_path_end \
is used to track if a fast tlab refill occured</span><o:p></o:p></pre> <pre><span \
class="m-452193571787093578m6512714288813904128m68639937780844273m8872727166074378132m-513143501369679108new"> \
46 // between \
slowpath calls.</span><o:p></o:p></pre> <p class="MsoNormal">There might be other \
comments too, I have not looked in detail.<o:p></o:p></p> </div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal">This was the only spot that still had an actual_end, I fixed it \
now. I'll do a sweep to double check other comments.<o:p></o:p></p> </div>
<div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<div>
<div>
<p class="MsoNormal"><br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p> </o:p></span></p>
</div>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">Not sure it's better but before updating the webrev, I \
wanted to try<br> to get input/consensus :)<br>
<br>
(Note hard_end was always further off than end).<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p \
class="MsoNormal">src/hotspot/share/prims/jvmti.xml:<br> <br>
10357 <capabilityfield id="can_sample_heap" \
since="9"><br> 10358 \
<description><br> 10359 Can \
sample the heap.<br> 10360 If this capability \
is enabled then the heap sampling methods<br> can be called.<br>
10361 </description><br>
10362 </capabilityfield><br>
<br>
Looks like this capability should not be "since 9" if it gets \
integrated<br> now.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">Updated now to 11, crossing my \
fingers :)<br> <br>
<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p \
class="MsoNormal">src/hotspot/share/runtime/heapMonitoring.cpp:<br> <br>
448 if (is_alive->do_object_b(value)) {<br>
449 // Update the oop to point to the new \
object if it is still<br> alive.<br>
450 f->do_oop(&(trace.obj));<br>
451<br>
452 // Copy the old trace, if it is still \
live.<br> 453 \
_allocated_traces->at_put(curr_pos++, trace);<br> 454<br>
455 // Store the live trace in a cache, to be \
served up on /heapz.<br> 456 \
_traces_on_last_full_gc->append(trace);<br> 457<br>
458 count++;<br>
459 } else {<br>
460 // If the old trace is no longer live, \
add it to the list of<br> 461 // recently \
collected garbage.<br> 462 \
store_garbage_trace(trace);<br> 463 }<br>
<br>
In the case where the oop was not live, I would like it to be explicitly<br>
cleared.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal">Done I think how you wanted it. Let me know because I'm not \
familiar<br> with the RootAccess API. I'm unclear if I'm doing this right or not \
so<br> reviews of these parts are highly appreciated. Robbin had talked of<br>
perhaps later pushing this all into a OopStorage, should I do this now<br>
do you think? Or can that wait a second webrev later down the road?<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt">I think using handles can and \
should be done later. You can use the Access API now.<br> I noticed that you are \
missing an #include "oops/access.inline.hpp" in your heapMonitoring.cpp \
file.<o:p></o:p></p> </blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt;background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222;background:white">The \
missing header is there for me so I don't know, I made sure it is present in the \
latest webrev. Sorry about that.</span><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"> <o:p></o:p></span></p>
</div>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">+ Did I clear it \
the way you wanted me to or were you thinking of<br> something else?<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
That is precisely how I wanted it to be cleared. Thanks.<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">+ Final question \
here, seems like if I were to want to not do the<br> f->do_oop directly on the \
trace.obj, I'd need to do something like:<br> <br>
f->do_oop(&value);<br>
...<br>
trace->store_oop(value);<br>
<br>
to update the oop internally. Is that right/is that one of the<br>
advantages of going to the Oopstorage sooner than later?<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
I think you really want to do the do_oop on the root directly. Is there a particular \
reason why you would not want to do that?<br> Otherwise, yes - the benefit with using \
the handle approach is that you do not need to call do_oop explicitly in your \
code.<o:p></o:p></p> </blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222">There \
is no reason except that now we have a load_oop and a get_oop_addr, I was not sure \
what you would think of that.<o:p></o:p></span></p> </div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</div>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p class="MsoNormal">That's fine.<br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal"><o:p> </o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Also I see a lot of \
concurrent-looking use of the following field:<br> 267 volatile \
bool _initialized;<br> <br>
Please note that the "volatile" qualifier does not help with reordering<br>
here. Reordering between volatile and non-volatile fields is completely free<br>
for both compiler and hardware, except for windows with MSVC, where volatile<br>
semantics is defined to use acquire/release semantics, and the hardware is<br>
TSO. But for the general case, I would expect this field to be stored with<br>
OrderAccess::release_store and loaded with OrderAccess::load_acquire.<br>
Otherwise it is not thread safe.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal">Because everything is behind a mutex, I wasn't really worried \
about<br> this. I have a test that has multiple threads trying to hit this<br>
corner case and it passes.<br>
<br>
However, to be paranoid, I updated it to using the OrderAccess API<br>
now, thanks! Let me know what you think there too!<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
If it is indeed always supposed to be read and written under a mutex, then I would \
strongly prefer to have it accessed as a normal non-volatile member, and have an \
assertion that given lock is held or we are in a safepoint, as we do in many other \
places. Something like this:<br>
<br>
assert(HeapMonitorStorage_lock->owned_by_self() || \
(SafepointSynchronize::is_at_safepoint() && \
Thread::current()->is_VM_thread()), "this should not be accessed \
concurrently");<br> <br>
It would be confusing to people reading the code if there are uses of OrderAccess \
that are actually always protected under a mutex.<o:p></o:p></p> </blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thank you for the exact example to be put in the code! I put it \
around each access/assignment of the _initialized method and found one case where yes \
you can touch it and not have the lock. It actually is "ok" because you \
don't act on the storage until later and only when you really want to modify the \
storage (see the object_alloc_do_sample method which calls the add_trace \
method).<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">But, because of this, I'm going to put the OrderAccess here, \
I'll do some performance numbers later and if there are issues, I might add a \
"unsafe" read and a "safe" one to make it explicit to the reader. \
But I don't think it will come to that.<o:p></o:p></p>
</div>
</div>
</div>
</div>
</blockquote>
<p class="MsoNormal"><br>
Okay. This double return in heapMonitoring.cpp looks wrong:<br>
<br>
283 bool initialized() {<br>
284 return OrderAccess::load_acquire(&_initialized) \
!= 0;<br> 285 return _initialized;<br>
286 }<br>
<br>
Since you said object_alloc_do_sample() is the only place where you do not hold the \
mutex while reading initialized(), I had a closer look at that. It looks like in its \
current shape, the lack of a mutex may lead to a memory leak. In particular, it first \
checks if (initialized()). Let's assume this is now true. It then allocates a bunch \
of stuff, and checks if the number of frames were over 0. If they were, it calls \
StackTraceStorage::storage()->add_trace() seemingly hoping that after grabbing the \
lock in there, initialized() will still return true. But it could now return false \
and skip doing anything, in which case the allocated stuff will never be \
freed.<o:p></o:p></p> </div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal">I fixed this now by making add_trace return a boolean and \
checking for that. It will be in the next webrev. Thanks, the truth is that in our \
implementation the system is always on or off, so this never really occurs :). In \
this version though, that is not true and it's important to handle so thanks \
again!<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal"><br>
So the analysis seems to be that _initialized is only used outside of the mutex in \
once instance, where it is used to perform double-checked locking, that actually \
causes a memory leak.<br> <br>
I am not proposing how to fix that, just raising the issue. If you still want to \
perform this double-checked locking somehow, then the use of acquire/release still \
seems odd. Because the memory ordering restrictions of it never comes into play in \
this particular case. If it ever did, then the use of destroy_stuff(); \
release_store(_initialized, 0) would be broken anyway as that would imply that \
whatever concurrent reader there ever was would after reading _initialized with \
load_acquire() could *never* read the data that is concurrently destroyed anyway. I \
would be biased to think that RawAccess<MO_RELAXED>::load/store looks like a \
more appropriate solution, given that the memory leak issue is resolved. I do not \
know how painful it would be to not perform this double-checked \
locking.<o:p></o:p></p> </div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">So I agree with this entirely. I looked also a bit more and the \
difference and code really stems from our internal version. In this version however, \
there are actually a lot of things going on that I did not go entirely through in my \
head but this comment made me ponder a bit more on it.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Since every object_alloc_do_sample is protected by a check to \
HeapMonitoring::enabled(), there is only a small chance that the call is happening \
when things have been disabled. So there is no real need to do a first check on the \
initialized, it is a rare occurence that a call happens to object_alloc_do_sample \
and the initialized of the storage returns false.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">(By the way, even if you did call object_alloc_do_sample without \
looking at HeapMonitoring::enabled(), that would be ok too. You would gather the \
stacktrace and get nowhere at the add_trace call, which would return false; so though \
not optimal performance wise, nothing would break).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Furthermore, the add_trace is really the moment of no return and \
we have the mutex lock and then the initialized check. So, in the end, I did two \
things: I removed that first check and then I removed the OrderAccess for the storage \
initialized. I think now I have a better grasp and understanding why it was done in \
our code and why it is not needed here. Thanks for pointing it out :). This now still \
passes my JTREG tests, especially the threaded one. <o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal"><br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<div>
<p class="MsoNormal" style="background:white"><span \
style="font-size:12.0pt;font-family:"Arial",sans-serif;color:#222222"><o:p> </o:p></span></p>
</div>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <blockquote \
style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">As a kind of meta comment, I wonder if it would make \
sense to add sampling<br> for non-TLAB allocations. Seems like if someone is rapidly \
allocating a<br> whole bunch of 1 MB objects that never fit in a TLAB, I might still \
be<br> interested in seeing that in my traces, and not get surprised that the<br>
allocation rate is very high yet not showing up in any profiles.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal">That is handled by the handle_sample where you wanted me to put \
a<br> UseTlab because you hit that case if the allocation is too big.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
I see. It was not obvious to me that non-TLAB sampling is done in the TLAB class. \
That seems like an abstraction crime.<br> What I wanted in my previous comment was \
that we do not call into the TLAB when we are not using TLABs. If there is sampling \
logic in the TLAB that is used for something else than TLABs, then it seems like that \
logic simply does not belong inside of the TLAB. It should be moved out of the TLAB, \
and instead have the TLAB call this common abstraction that makes \
sense.<o:p></o:p></p> </blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">So in the incremental version:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.07_08/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.07_08/</a>, this \
is still a "crime". The reason is that the system has to have the \
bytes_until_sample on a per-thread level and it made "sense" to have it \
with the TLAB implementation. Also, I was not sure how people felt about adding \
something to the thread instance instead.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Do you think it fits better at the Thread level? I can see how \
difficult it is to make it happen there and add some logic there. Let me know what \
you think.<o:p></o:p></p> </div>
</div>
</div>
</div>
</blockquote>
<p class="MsoNormal"><br>
We have an unfortunate situation where everyone that has some fields that are thread \
local tend to dump them right into Thread, making the size and complexity of Thread \
grow as it becomes tightly coupled with various unrelated subsystems. It would be \
desirable to have a separate class for this instead that encapsulates the sampling \
logic. That class could possibly reside in Thread though as a value object of \
Thread.<o:p></o:p></p> </div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I imagined that would be the case but was not sure. I will look \
at the example that Robbin is talking about (ThreadSMR) and will see how to refactor \
my code to use that.<o:p></o:p></p> </div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thanks again for your help,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <div>
<p class="MsoNormal"><br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">Hope I have answered your questions and that my feedback \
makes sense to you.<o:p></o:p></p> </blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">You have and thank you for them, I think we are getting to a \
cleaner implementation and things are getting better and more readable \
:)<o:p></o:p></p> </div>
</div>
</div>
</div>
</blockquote>
<p class="MsoNormal"><br>
Yes it is getting better.<br>
<br>
Thanks,<br>
/Erik<o:p></o:p></p>
<div>
<div>
<p class="MsoNormal"><br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<p class="MsoNormal">Thanks for your help!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Thanks,<br>
/Erik <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">I double checked by changing the test<br> <a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.05a/raw_files/new/test/hot \
spot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.java" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05a/raw_files/new/ \
test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.java</a><br>
<br>
to use a smaller Tlab (2048) and made the object bigger and it goes<br>
through that and passes.<br>
<br>
Thanks again for your review and I look forward to your pointers for<br>
the questions I now have raised!<br>
Jc<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Thanks,<br>
/Erik<br>
<br>
<br>
On 2018-01-26 06:45, JC Beyler wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">Thanks Robbin for the \
reviews :)<br> <br>
The new full webrev is here:<br>
<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.03/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.03/</a><br> The \
incremental webrev is here:<br> <a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.02_03/" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02_03/</a><br> \
<br> I inlined my answers:<br>
<br>
On Thu, Jan 25, 2018 at 1:15 AM, Robbin Ehn <<a \
href="mailto:robbin.ehn@oracle.com" target="_blank">robbin.ehn@oracle.com</a>> \
wrote:<o:p></o:p></p> <blockquote style="border:none;border-left:solid #CCCCCC \
1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in"> <p \
class="MsoNormal">Hi JC, great to see another revision!<br> <br>
####<br>
heapMonitoring.cpp<br>
<br>
StackTraceData should not contain the oop for 'safety' reasons.<br>
When StackTraceData is moved from _allocated_traces:<br>
L452 store_garbage_trace(trace);<br>
it contains a dead oop.<br>
_allocated_traces could instead be a tupel of oop and StackTraceData thus<br>
dead oops are not kept.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">Done I used inheritance to make the \
copier work regardless but the<br> idea is the same.<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">You should use the \
new Access API for loading the oop, something like<br> this:<br>
RootAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::load(...)<br>
I don't think you need to use Access API for clearing the oop, but it<br>
would<br>
look nicer. And you shouldn't probably be using:<br>
Universe::heap()->is_in_reserved(value)<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">I am unfamiliar with this but I \
think I did do it like you wanted me<br> to (all tests pass so that's a start). I'm \
not sure how to clear the<br> oop exactly, is there somewhere that does that, which I \
can use to do<br> the same?<br>
<br>
I removed the is_in_reserved, this came from our internal version, I<br>
don't know why it was there but my tests work without so I removed it<br>
> )<br>
<br>
<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal">The lock:<br>
L424 MutexLocker mu(HeapMonitorStorage_lock);<br>
Is not needed as far as I can see.<br>
weak_oops_do is called in a safepoint, no TLAB allocation can happen and<br>
JVMTI thread can't access these data-structures. Is there something more<br>
to<br>
this lock that I'm missing?<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">Since a thread can call the JVMTI \
getLiveTraces (or any of the other<br> ones), it can get to the point of trying to \
copying the<br> _allocated_traces. I imagine it is possible that this is \
happening<br> during a GC or that it can be started and a GC happens afterwards.<br>
Therefore, it seems to me that you want this protected, no?<br>
<br>
<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">####<br> You have 6 files without any changes in them \
(any more):<br> g1CollectedHeap.cpp<br>
psMarkSweep.cpp<br>
psParallelCompact.cpp<br>
genCollectedHeap.cpp<br>
referenceProcessor.cpp<br>
thread.hpp<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">Done.<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">####<br> I have not looked closely, but is it possible \
to hide heap sampling in<br> AllocTracer ? (with some minor changes to the \
AllocTracer API)<o:p></o:p></p> </blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">I am imagining that you are saying \
to move the code that does the<br> sampling code (change the tlab end, do the call to \
HeapMonitoring,<br> etc.) into the AllocTracer code itself? I think that is right and \
I'll<br> look if that is possible and prepare a webrev to show what would be<br>
needed to make that happen.<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">####<br> Minor nit, when declaring pointer there is a \
little mix of having the<br> pointer adjacent by type name and data name. (Most \
hotspot code is by<br> type<br>
name)<br>
E.g.<br>
heapMonitoring.cpp:711 jvmtiStackTrace *trace = ....<br>
heapMonitoring.cpp:733 Method* m = \
vfst.method();<br> (not just this file)<o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt">Done!<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in \
6.0pt;margin-left:4.8pt;margin-right:0in"> <p class="MsoNormal" \
style="margin-bottom:12.0pt">####<br> HeapMonitorThreadOnOffTest.java:77<br>
I would make g_tmp volatile, otherwise the assignment in loop may<br>
theoretical be skipped.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal">Also done!<br>
<br>
Thanks again!<br>
Jc<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</blockquote>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic