<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;">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"><span><div><div></div><div>Hi Tony,

<br>

<br>On Fri, 2016-01-29 at 12:12 -0500, Tony Printezis wrote:

<br>> Thomas,

<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.  

> 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

> 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

> 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

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

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.

> (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

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

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>

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

> 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

time on it.

<br>

<br>Thanks,

<br>  Thomas

<br>

<br></div></div></span></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></body></html>