<html><head><style>body{font-family:Helvetica,Arial;font-size:13px}</style></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Updated webrev with a small change suggested by John Coomes (CCed) to move the in_c_heap arg to the c’tor instead of the init() / reclaim() methods:</div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br></div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><a href="http://cr.openjdk.java.net/~tonyp/8146989/webrev.3/">http://cr.openjdk.java.net/~tonyp/8146989/webrev.3/</a></div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br></div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Tony</div> <br><p class="airmail_on">On February 3, 2016 at 1:34:15 PM, Tony Printezis (<a href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a>) wrote:</p> <blockquote type="cite" class="clean_bq"><span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><div>
<title></title>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
Thomas,</div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<br></div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
Is that what you were looking for?</div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<br></div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<a href="http://cr.openjdk.java.net/~tonyp/8146989/webrev.2/">http://cr.openjdk.java.net/~tonyp/8146989/webrev.2/</a></div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<br></div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
I haven’t included the PreservedMarks / PreservedMarksSet
abstractions in ParallelOld yet, but I browed the code and it will
be straightforward to do so (I implemented them with that in
mind…).</div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<br></div>
<div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
Tony</div>
<br>
<p class="airmail_on">On February 2, 2016 at 6:57:37 AM, Thomas
Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>)
wrote:</p>
<blockquote type="cite" class="clean_bq">
<div>
<div><span>Hi Tony,<br>
<br>
On Fri, 2016-01-29 at 12:12 -0500, Tony Printezis wrote:<br>
> Thomas,<br>
><br>
> Apologies for the delay. Inline.<br>
><br>
> > On January 25, 2016 at 11:24:01 AM, Thomas Schatzl
(<br>
> > thomas.schatzl@oracle.com) wrote:Hi Tony,<br>
> ><br>
> > On Fri, 2016-01-22 at 16:59 -0200, Tony Printezis
wrote:<br>
> > > Hi Thomas,<br>
<br>
[...]<br>
> > > * Different GCs have different assumptions on the
lifetime of the<br>
> > > data structures they allocate for their workers.
(From memory)<br>
> > > ParallelGC allocates them once upfront and re-uses
them at every<br>
> > > GC, ParNew allocates them before every GC in an
arena and<br>
> > > reclaims them at the end of the GC (in the
ResourceMark d’tor<br>
> > > IIRC), etc. So, IMHO, it will be easier to just
embed a<br>
> > > PreservedMarks field in the worker state (as I did
in the webrev)<br>
> > > so that its lifetime / reclamation is<br>
> > > dealt with automatically and according to the GC’s
assumptions.<br>
> ><br>
> > Not sure if this exactly corresponds to my understanding
of the<br>
> > lifetime of ParScanThreadState (PSS in the following): to
me it is<br>
> > a helper data structure that is for storing information
that comes<br>
> > up during the evacuation phase only.<br>
> This should have been ‘arrays', not ‘array'...<br>
<br>
<br>
> Sure. And both ParallelGC and G1 have equivalents. But the
lifetime<br>
> of those data structures for each GC is different. For
example, the<br>
> instances of ParScanThreadState are allocated in the resource
arena<br>
> at the start of each ParNew (see the c’tor of
ParScanThreadStateSet<br>
> in parNewGeneration.cpp) and reclaimed at the end of the
ParNew, the<br>
> instances of PSPromotionManager are allocated during JVM<br>
> initialization (see PSPromotionManager::initialize() in<br>
> psPromotionManager.cpp) and re-used at every GC, etc.
Personally, I<br>
> like the idea of adding a PreservedMarks field the GC worker
state<br>
> classes and piggy-back on whatever memory management they do.
If you<br>
> want to do something different, and manage the
PreservedMarks<br>
> separately, you’ll now have two data structures with
different<br>
> lifetimes interacting with each other.<br>
<br>
> > Again to me, preserved mark handling/restoration, also
because it<br>
> > needs to be done in a separate parallel phase after
main<br>
> > evacuation, has a different lifetime and so its owner
should not<br>
> > be the PSS.<br>
> Personally, I don’t think it does. There’s some clean up to be
done<br>
> at the end of the GC that needs to interact with the GC worker
state<br>
> instances (e.g., flush PLABs). Doing preserved mark
restoration as<br>
> part of that process is not unreasonable, IMHO.<br>
<br>
Well, similar arguments could be said why other data structures
that<br>
are ultimately filled in with information from the PSS are not in
the<br>
PSS.<br>
<br>
I can see your point, but I hope we can agree to disagree on this
right<br>
now. Let's just leave this change out now unless somebody else has
a<br>
strong opinion.<br>
<br>
<br>
> > [At least for G1 we might look into that, i.e. using
different<br>
> > amount of threads for different parallel phases for
various reasons<br>
> > quite soon.<br>
> (Out of curiosity) Do you have a CR for this?<br>
<br>
Multiple (I am including some related to parallization here):<br>
<br>
8034842: Parallelize the Free CSet phase in G1<br>
7068229: G1: Dynamically enable MT reference processing for
remark<br>
pauses<br>
8043575: Dynamically parallelize reference processing work<br>
8143024: Make aggregate-data phase concurrent<br>
8133051: Concurrent refinement threads may be activated and
deactivated<br>
at random<br>
8076462: Preserving the referents of concurrent mark work
distribution<br>
method causes long pause times<br>
8057000: Increase parallelism in String deduplication fixup<br>
8040006: Merge "Other" time parallel phases into a single<br>
<br>
Note that these are not complete, and just what I found quickly
when<br>
browsing through issues with the g1-gc label.<br>
<br>
Also, apart from the general hint that we are aware of problems in
a<br>
particular area, there may be some changes in other components for
jdk9<br>
that might obsolete some of these CRs, or invalidate the
approach<br>
indicated.<br>
<br>
So if you want to spend time on one or another problem, please give
a<br>
heads up. :)<br>
<br>
> > - it is known where the mark restoration code is located.
(exactly<br>
> > in<br>
> > PreservedMarksSet::restore_marks()), and not in more or
less<br>
> > randomly<br>
> > named methods depending on collector.<br>
> This is a non-requirement, IMHO. Most of the work is done in
the<br>
> restore() method of PreservedMarks. Where that’s called from I
don’t<br>
> think it matters, IMHO.<br>
<br>
Again, just a matter of taste - it is much easier to understand
stuff<br>
if things work similarly and are used in a similar way in the
different<br>
collectors. Otherwise there is always a certain additional
reluctance<br>
to dig into e.g. CMS code for some particular purpose just because
the<br>
same things are already named quite differently, and the same
things<br>
are done very differently. (I am not advocating a
try-to-share<br>
-everything-everywhere approach here; but consistent naming and use
of<br>
the same concepts in the same way would decrease that hurdle
:))<br>
<br>
It is not a big issue. Let's leave this refactoring here
then.<br>
<br>
> > - makes the main evacuation method for all collectors a
bit more<br>
> > uniform. I.e. all do<br>
> ><br>
> > evacuate_collection_set();<br>
> > if (evacuation_failed()) {<br>
> > PreservedMarksSet::restore_marks()<br>
> > }<br>
> ><br>
> > - puts mark restoration code in subclasses (searching
for<br>
> > subclasses of<br>
> > PreservedMarksStack is much easier for me<br>
> …and it makes the code more complicated to follow, as you
first need<br>
> to know which subclass is used in each GC. I generally
prefer<br>
> straight-line code, than having to understand a class
hierarchy.<br>
<br>
:)<br>
<br>
> Anyway, I need to get this off my plate sooner than later. If
you<br>
> still want me to do the PreservedMarksSet implementation, I
can. But<br>
> tell me how to manage the memory for the PreservedMarks
instances. Do<br>
> I malloc them on the C-heap at the start of GC and reclaim
them at<br>
> the end?<br>
<br>
I am fine with the current approach if you do not feel like
spending<br>
time on it.<br>
<br>
Thanks,<br>
Thomas<br>
<br></span></div>
</div>
</blockquote>
<div id="bloop_sign_1454524145805405184" class="bloop_sign">
<div style="font-family:helvetica,arial;font-size:13px">
<div>-----</div>
<div><br></div>
<div>Tony Printezis | JVM/GC Engineer / VM Team | Twitter</div>
<div><br></div>
<div>@TonyPrintezis</div>
<div><a href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></div>
<div><br></div>
</div>
</div>
</div></div></span></blockquote> <div id="bloop_sign_1454681783886738944" class="bloop_sign"><div style="font-family:helvetica,arial;font-size:13px"><div>-----</div><div><br></div><div>Tony Printezis | JVM/GC Engineer / VM Team | Twitter</div><div><br></div><div>@TonyPrintezis</div><div><a href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></div><div><br></div></div></div></body></html>