<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi,<br>
<br>
After some further discussion we the GC team we have decided to
split up this patch further to make it easier to push.<br>
Since this patch covers all GCs and multiple changes are on their
way in creating conflicts in the different GCs it is hard to push
this in a single commit.<br>
So to make sure this moves forward and reduce contention we
decided to split it up in 4 pieces.<br>
<br>
1. Event Implementation<br>
2. Support for PS and ParOld<br>
3. Support for CMS<br>
4. Support for G1<br>
<br>
I have created sub-tasks for each of these in the JDK-8055845.<br>
<br>
I'm working on the first step and will send out a separate review
for that later today and the others will follow later.<br>
<br>
Cheers,<br>
Staffan<br>
<br>
On 11/19/2014 08:23 AM, Staffan Friberg wrote:<br>
</div>
<blockquote cite="mid:546CC3FE.7070105@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi,<br>
<br>
Erik and Tomas asked me to separate the G1 changes which was
pushed separately in JDK-8064473.<br>
Erik also had some further comments offline that I have taken
care of.<br>
<br>
Here is the new webrev,
cr.openjdk.java.net/~sfriberg/8055845/webrev.06<br>
<br>
Thanks,<br>
Staffan<br>
<br>
<br>
On 11/06/2014 01:53 PM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:545B6F58.2000907@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
<br>
<div class="moz-cite-prefix">On 2014-11-06 14:00, Staffan
Friberg wrote:<br>
</div>
<blockquote cite="mid:545B70D2.9080008@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Good catch, fixed.<br>
<br>
// Too large; allocate the object individually.<br>
obj = sp->par_allocate(word_sz);<br>
if (obj != NULL) {<br>
gc_tracer()->report_promotion_outside_plab_event(old,
word_sz, age, false);<br>
}<br>
<br>
Please let me know if anyone wants a full new webrev with
this.<br>
</div>
</blockquote>
<br>
<br>
Looks good. Reviewed.<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:545B70D2.9080008@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Staffan<br>
<br>
<br>
On 11/06/2014 12:01 PM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:545B5520.7010407@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
Hi Staffan,<br>
<br>
<div class="moz-cite-prefix">On 2014-11-06 11:12, Staffan
Friberg wrote:<br>
</div>
<blockquote cite="mid:545B4986.1050700@oracle.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi,<br>
<br>
After further off list discussion it was decided to keep
the gc_tracer in par_promote as is.<br>
<br>
I have uploaded a new webrev, <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esfriberg/8055845/webrev.05">http://cr.openjdk.java.net/~sfriberg/8055845/webrev.05</a><br>
<br>
The main change here is a rewrite of the G1 code which
is cleaner and also reuses the read age. By sending the
markOop down through the call we can now trust the read
age and do not need to reread it when incrementing which
improves the YC performance slightly as it avoids the
rather complex bit extraction.<br>
</div>
</blockquote>
<br>
<br>
Looks good to me. One detail in parNewGeneration.cpp:<br>
<br>
274 } else {<br>
275 // Too large; allocate the object individually.<br>
276
gc_tracer()->report_promotion_outside_plab_event(old,
word_sz, age, false);<br>
277 obj = sp->par_allocate(word_sz);<br>
278 }<br>
<br>
Seems like par_allocate() return NULL. Maybe we should check
that before reporting the event. Similarly to what you do in
the other GCs, for example G1:<br>
<br>
g1Allocator.cpp<br>
<br>
141 obj = _g1h->par_allocate_during_gc(purpose,
word_sz, context);<br>
142 if (obj != NULL<br>
143 &&
_g1h->_gc_tracer_stw->should_report_promotion_outside_plab_event())
{<br>
144 bool tenured =
_g1h->heap_region_containing_raw(obj)->is_old();<br>
145
_g1h->_gc_tracer_stw->report_promotion_outside_plab_event(old,
word_sz, <br>
146
age, tenured);<br>
147 }<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:545B4986.1050700@oracle.com"
type="cite">
<div class="moz-cite-prefix"> <br>
Cheers,<br>
Staffan<br>
<br>
On 09/15/2014 02:06 PM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:5416D64C.9020207@oracle.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
Hi Staffan,<br>
<br>
psPromotionManager.inline.hpp<br>
<br>
I think the PSPromotionManager::copy_to_survivor_space()
might send multiple events. If the allocation to the
young gen fails we will fall through to do an old gen
allocation. But we send the events before we realize
that the allocation has failed, i.e. new_obj is NULL.<br>
<br>
I talked to Erik a bit about how to handle the gc_tracer
in par_promote. He'll get back to you with some thoughts
on that.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<div class="moz-cite-prefix">On 2014-09-06 00:20,
Staffan Friberg wrote:<br>
</div>
<blockquote cite="mid:540A3730.1060603@oracle.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi,<br>
<br>
I have uploaded a new webrev here,
cr.openjdk.java.net/~sfriberg/8055845/webrev.03<br>
<br>
It contains several changes<br>
<br>
- Split event into two events
(PromoteObjectInNewPLAB, PromoteObjectOutsidePLAB)<br>
- Moved events to
"vm/gc/detailed/PromoteObject..."<br>
- Supporting ParNew+CMS and ParNew+SerialOld
tenuring<br>
- Not sure if the way I do it with passing
the ParNewTracer is the best solution, please let me
know if you have an idea how to improve it<br>
- Simplified the G1 code to avoid sending age
and having a single call site<br>
- Fixed so that the generated event has size
information in bytes rather than words<br>
<br>
Thanks for offline comments and suggestions from
Dmitry and Thomas.<br>
<br>
Cheers,<br>
Staffan<br>
<br>
On 08/29/2014 03:32 PM, Staffan Friberg wrote:<br>
</div>
<blockquote cite="mid:5400FF6F.5040203@oracle.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Erik,<br>
<br>
On 08/28/2014 11:34 PM, Erik Helin wrote:<br>
</div>
<blockquote cite="mid:54001EFF.3030802@oracle.com"
type="cite">(it seems like we lost <a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>
somewhere in this thread, I'm adding it back) <br>
<br>
On 2014-08-28 23:15, Staffan Friberg wrote: <br>
<blockquote type="cite">Hi Erik, <br>
<br>
Thanks for the comments. <br>
<blockquote type="cite">- Aren't the events for
promotion to the tenured generation
(SerialOld) <br>
and the CMS generation missing? <br>
</blockquote>
The reason for leaving out these two, Serial
completely and CMS <br>
promotion, was due to that neither as far as I
understand make use of <br>
PLABs. <br>
</blockquote>
<br>
I might be wrong here, but looking at the function
TenuredGeneration::par_promote (in
tenuredGeneration.cpp) it looks to me like
SerialOld is using PLABs when ParNew is promoting
objects from young to old. <br>
<br>
As for CMS, looking at
ConcurrentMarkSweepGeneration::par_promote (in
concurrentMarkSweepGeneration.cpp) it seems like
each CMSParGCThreadState has a CFLS_LAB
(CompactibleFreeListSpace Local Allocation Buffer)
that is a thread-local allocation buffer. See
compactibleFreeListSpace.{hpp,cpp} for more
details. <br>
<br>
Given this, I think we should add events for
Serial and CMS as well. <br>
<br>
</blockquote>
<br>
Ok I see what you mean with CMS, basically the
equivalent to getting a PLAB would be when we refill
the CFLS_LAB in the alloc function. It still does
the allocation to a small memory (~ size of object)
area from the freelist, but at least we did extra
work to refill the local CFLS_LAB. Need to do some
analysis to see how often we refill so the overhead
doesn't get too high.<br>
The only issue I run into is how I can in a nice way
get access to the ParNewTracer from ParNewGeneration
to call the report function. Let's sync up next week
and see how it can be solved.<br>
<br>
The tenured GC requires something similar to be
supported, however since ParNewGC is deprecated for
usage without CMS in JDK 8 we might want to skip
that combination.<br>
<br>
<br>
<blockquote cite="mid:54001EFF.3030802@oracle.com"
type="cite"> <br>
On 2014-08-28 23:15, Staffan Friberg wrote: <br>
<blockquote type="cite">
<blockquote type="cite">- Would it make sense to
differentiate, either by separate events or by
<br>
a field in a event, between promotions to
to-space and to the old <br>
generation? <br>
- The are two events for TLAB allocations, <br>
java/object_alloc_in_new_TLAB and
java/object_alloc_outside_TLAB. <br>
What do you think about using two events for
PLAB allocations as well: <br>
- java/object_alloc_in_new_PLAB <br>
- java/object_alloc_outside_PLAB <br>
</blockquote>
I think this is a matter of taste and probably
how similar we want the <br>
event to be to the existing allocation event. I
personally prefer a <br>
single event but if the GC team and
serviceability team feel it would be <br>
better to have two I can certainly rewrite. The
reason for me preferring <br>
a single event is just ease of analysis, you can
easily filter a list of <br>
events on a field, it is harder to merge two
different events with <br>
different fields and work with them in an
straight forward fashion. <br>
<br>
Any general preference for having a single or
multiple events? <br>
</blockquote>
<br>
I would prefer to have two events in this case and
try to follow the existing allocation events as
much as possible (both in naming and in style).
Keeping the tenured field (I missed it the first
time I read the patch) is good. <br>
<br>
</blockquote>
Yes, tenured would be independent of having two
events, only PLAB size and directAllocation would be
affected when having two event types.<br>
<br>
<b>Erik Gahlin</b>, any preference from your end?<br>
<br>
<br>
<br>
<blockquote cite="mid:54001EFF.3030802@oracle.com"
type="cite">On 2014-08-28 23:15, Staffan Friberg
wrote: <br>
<blockquote type="cite">
<blockquote type="cite">- In PSPromotionManager,
instead of utilizing the C++ friendship with <br>
PSScavenge, should we add a getter function
for the gc_tracer? <br>
</blockquote>
Created a getter function. <br>
</blockquote>
<br>
Thanks! If you make report_promotion_sample const,
then the getter can return a const
ParallelScavengeTracer*, right? <br>
<br>
</blockquote>
Done<br>
<br>
//Staffan<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>