
<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;">Apologies for the delay. Inline.</div> <br><p class="airmail_on">On January 25, 2016 at 11:24:01 AM, Thomas Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>) wrote:</p> <div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div></div><div>Hi Tony,<span class="Apple-converted-space"> </span><br><br>On Fri, 2016-01-22 at 16:59 -0200, Tony Printezis wrote:<span class="Apple-converted-space"> </span><br>> Hi Thomas,<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> Thanks for looking at it. I’m traveling this week (at the JCP EC<span class="Apple-converted-space"> </span><br>> meeting) so I’ll look at this in more detail early next week.<span class="Apple-converted-space"> </span><br><br>No problem.<span class="Apple-converted-space"> </span><br><br>> To make sure I understand: So, you’re suggesting that I introduce an<span class="Apple-converted-space"> </span><br>> abstraction that keeps track of the PreservedMarks array (either as a<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>This should have been ‘arrays', not ‘array'...</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>> separate class, PreservedMarksSet, or maybe as a static field in the<span class="Apple-converted-space"> </span><br>> PreservedMarks class) so that the logic on how to restore the<span class="Apple-converted-space"> </span><br>> preserved marks is shared instead of replicated. I think there are a<span class="Apple-converted-space"> </span><br>> couple of issues with this:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> * Different GCs have different assumptions on the lifetime of the<span class="Apple-converted-space"> </span><br>> data structures they allocate for their workers. (From memory)<span class="Apple-converted-space"> </span><br>> ParallelGC allocates them once upfront and re-uses them at every GC,<span class="Apple-converted-space"> </span><br>> ParNew allocates them before every GC in an arena and reclaims them<span class="Apple-converted-space"> </span><br>> at the end of the GC (in the ResourceMark d’tor IIRC), etc. So, IMHO,<span class="Apple-converted-space"> </span><br>> it will be easier to just embed a PreservedMarks field in the worker<span class="Apple-converted-space"> </span><br>> state (as I did in the webrev) so that its lifetime / reclamation is<span class="Apple-converted-space"> </span><br>> dealt with automatically and according to the GC’s assumptions.<span class="Apple-converted-space"> </span><br><br>Not sure if this exactly corresponds to my understanding of the<span class="Apple-converted-space"> </span><br>lifetime of ParScanThreadState (PSS in the following): to me it is a<span class="Apple-converted-space"> </span><br>helper data structure that is for storing information that comes up<span class="Apple-converted-space"> </span><br>during the evacuation phase only.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Sure. And both ParallelGC and G1 have equivalents. But the lifetime of those data structures for each GC is different. For example, the instances of ParScanThreadState are allocated in the resource arena at the start of each ParNew (see the c’tor of ParScanThreadStateSet in parNewGeneration.cpp) and reclaimed at the end of the ParNew, the instances of PSPromotionManager are allocated during JVM initialization (see PSPromotionManager::initialize() in psPromotionManager.cpp) and re-used at every GC, etc. Personally, I like the idea of adding a PreservedMarks field the GC worker state classes and piggy-back on whatever memory management they do. If you want to do something different, and manage the PreservedMarks separately, you’ll know have two data structures with different lifetimes interacting with each other.</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>Again to me, preserved mark handling/restoration, also because it needs<span class="Apple-converted-space"> </span><br>to be done in a separate parallel phase after main evacuation, has a<span class="Apple-converted-space"> </span><br>different lifetime and so its owner should not be the PSS.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Personally, I don’t think it does. There’s some clean up to be done at the end of the GC that needs to interact with the GC worker state instances (e.g., flush PLABs). Doing preserved mark restoration as part of that process is not unreasonable, IMHO.</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>The other thing is, the threads restoring the marks may be different<span class="Apple-converted-space"> </span><br>ones than the ones pushing marks onto them. I.e. use a different amount<span class="Apple-converted-space"> </span><br>of threads for mark restoration than evacuation to better balance the<span class="Apple-converted-space"> </span><br>(available) workload. Then having the PSS the "owner" of the stacks<span class="Apple-converted-space"> </span><br>just gets in the way.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Each GC already has an array that keeps track of the GC worker state instances. So you can always grab it from there, you don’t need to have an 1-1 relationship between GC worker and GC worker state. (FWIW)</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>[At least for G1 we might look into that, i.e. using different amount<span class="Apple-converted-space"> </span><br>of threads for different parallel phases for various reasons quite<span class="Apple-converted-space"> </span><br>soon.</div></div></span></blockquote></div><p><br></p><p>(Out of curiosity) Do you have a CR for this?</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><span class="Apple-converted-space"> </span>Which means, please keep the preserved mark stacks separate from<span class="Apple-converted-space"> </span><br>its PSS in at least G1 :]<span class="Apple-converted-space"> </span><br><br>> * I do like the idea of having the code that restores the preserved<span class="Apple-converted-space"> </span><br>> marks (maybe serially, maybe in parallel) abstracted away. But, each<span class="Apple-converted-space"> </span><br>> GC maintains its own pool of worker threads and also decides how many<span class="Apple-converted-space"> </span><br>> workers it uses at every GC (can this still be calculated<span class="Apple-converted-space"> </span><br>> dynamically?). I think this will result in a slightly messy GC<span class="Apple-converted-space"> </span><br>> -PreservedMarksSet interface.<span class="Apple-converted-space"> </span><br><br>The main idea of this unified marks restoration method would be that it<span class="Apple-converted-space"> </span><br><br>- it is known where the mark restoration code is located. (exactly in<span class="Apple-converted-space"> </span><br>PreservedMarksSet::restore_marks()), and not in more or less randomly<span class="Apple-converted-space"> </span><br>named methods depending on collector.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>This is a non-requirement, IMHO. Most of the work is done in the restore() method of PreservedMarks. Where that’s called from I don’t think it matters, IMHO. </p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>- makes the main evacuation method for all collectors a bit more<span class="Apple-converted-space"> </span><br>uniform. I.e. all do<span class="Apple-converted-space"> </span><br><br>evacuate_collection_set();<span class="Apple-converted-space"> </span><br>if (evacuation_failed()) {<span class="Apple-converted-space"> </span><br>PreservedMarksSet::restore_marks()<span class="Apple-converted-space"> </span><br>}<span class="Apple-converted-space"> </span><br><br>- puts mark restoration code in subclasses (searching for subclasses of<span class="Apple-converted-space"> </span><br>PreservedMarksStack is much easier for me</div></div></span></blockquote></div><p><br></p><p>…and it makes the code more complicated to follow, as you first need to know which subclass is used in each GC. I generally prefer straight-line code, than having to understand a class hierarchy.</p><p>Anyway, I need to get this off my plate sooner than later. If you still want me to do the PreservedMarksSet implementation, I can. But tell me how to manage the memory for the PreservedMarks instances. Do I malloc them on the C-heap at the start of GC and reclaim them at the end?</p><p>Tony</p><p><br></p><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><span class="Apple-converted-space"> </span>than guessing the name of the<span class="Apple-converted-space"> </span><br>day for a particular collector) instead of putting it somewhere in<span class="Apple-converted-space"> </span><br>CollectedHeap instances. You could then also remove some mark<span class="Apple-converted-space"> </span><br>restoration helper types out of the global namespace.<span class="Apple-converted-space"> </span><br><br>> * Customizing restore_all_marks() for each GC to address some of the<span class="Apple-converted-space"> </span><br>> above issues will defeat the purpose of introducing the abstraction<span class="Apple-converted-space"> </span><br>> in the first place.<span class="Apple-converted-space"> </span><br><br>I am aware that the actual code for restoring the marks can not be<span class="Apple-converted-space"> </span><br>shared. I see it as a way to improve readability of the code.<span class="Apple-converted-space"> </span><br><br>> So, even though I understand the attraction of doing what you’re<span class="Apple-converted-space"> </span><br>> suggesting, it will be more trouble that it’s worth IMHO. And note<span class="Apple-converted-space"> </span><br>> that my change doesn’t add more duplicated code (and it will actually<span class="Apple-converted-space"> </span><br>> decrease the amount of duplicated code when we use PreservedMarks in<span class="Apple-converted-space"> </span><br>> ParallelGC). The code is already duplicated.<span class="Apple-converted-space"> </span><br><br>I do not insist implementing this the way I would do that. That idea<span class="Apple-converted-space"> </span><br>may be done at a different time, or altogether scrapped.<span class="Apple-converted-space"> </span><br><br>> Re: long notes: Feel free to delete them. :-) Of course we can create<span class="Apple-converted-space"> </span><br>> CRs for some of this follow-up. But when someone is reading the code<span class="Apple-converted-space"> </span><br>> they will not go and look up CRs if they wonder about something, so<span class="Apple-converted-space"> </span><br>> it’s helpful to have some explanation as a comment.<span class="Apple-converted-space"> </span><br><br>Re-reading it again, it's fine with me to keep that note after all. I<span class="Apple-converted-space"> </span><br>seem to have misunderstood the purpose of this wall of text :)<span class="Apple-converted-space"> </span><br><br>> I’ll fix the copyright dates.<span class="Apple-converted-space"> </span><br><br>Thanks,<span class="Apple-converted-space"> </span><br>Thomas<span class="Apple-converted-space"> </span><br><br></div></div></span></blockquote><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div> <div id="bloop_sign_1454078239724569088" 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>