RFR: 8146989: Introduce per-worker preserved mark stacks in ParNew

Fri Jan 22 18:59:21 UTC 2016

Hi Thomas,

Thanks for looking at it. I’m traveling this week (at the JCP EC meeting) so I’ll look at this in more detail early next week.

To make sure I understand: So, you’re suggesting that I introduce an abstraction that keeps track of the PreservedMarks array (either as a separate class, PreservedMarksSet, or maybe as a static field in the PreservedMarks class) so that the logic on how to restore the preserved marks is shared instead of replicated. I think there are a couple of issues with this:

* Different GCs have different assumptions on the lifetime of the data structures they allocate for their workers. (From memory) ParallelGC allocates them once upfront and re-uses them at every GC, ParNew allocates them before every GC in an arena and reclaims them at the end of the GC (in the ResourceMark d’tor IIRC), etc. So, IMHO, it will be easier to just embed a PreservedMarks field in the worker state (as I did in the webrev) so that its lifetime / reclamation is dealt with automatically and according to the GC’s assumptions.

* I do like the idea of having the code that restores the preserved marks (maybe serially, maybe in parallel) abstracted away. But, each GC maintains its own pool of worker threads and also decides how many workers it uses at every GC (can this still be calculated dynamically?). I think this will result in a slightly messy GC-PreservedMarksSet interface.

* Customizing restore_all_marks() for each GC to address some of the above issues will defeat the purpose of introducing the abstraction in the first place.

So, even though I understand the attraction of doing what you’re suggesting, it will be more trouble that it’s worth IMHO. And note that my change doesn’t add more duplicated code (and it will actually decrease the amount of duplicated code when we use PreservedMarks in ParallelGC). The code is already duplicated.

Re: long notes: Feel free to delete them. :-) Of course we can create CRs for some of this follow-up. But when someone is reading the code they will not go and look up CRs if they wonder about something, so it’s helpful to have some explanation as a comment.

I’ll fix the copyright dates.

Tony

On January 19, 2016 at 11:42:18 AM, Thomas Schatzl (thomas.schatzl at oracle.com) wrote:

Hi,  

sorry for the delay.  

On Wed, 2016-01-13 at 19:07 -0500, Tony Printezis wrote:  
> This is the change that introduces per-worker preserved mark stacks  
> for ParNew (please see the previous thread on this):  
>  
> http://cr.openjdk.java.net/~tonyp/8146989/webrev.1/  

- I would prefer if the code were structured differently. I do not like  
the additional remove_forwarding_pointers() in ParNewGeneration, and  
the call to DefNew::remove_forwarding_pointers() in the way it's done  
right now.  

Considering how mark restoration will likely be implemented in the  
other collectors (like in  
ParNewGeneration::remove_forwarding_pointers()), it looks like there  
will be a lot of code duplication involved.  

So my suggestion is to add a PreservedMarksSet class that is as the  
name implies basically a set of PreservedMarks.  

Apart from getting references to the individual PreservedMarks (to pass  
to ParScanThreadState), it has one virtual method that restores the  
marks. In the default implementation, it just iterates over all  
PreservedMarks elements of the set, calling PreservedMark::restore().  

Add one particular instance to DefNewGeneration - in case of ParNew  
usage initialize it with #parallel threads PreservedMarks, in case of  
DefNew just one.  

Pass PreservedMarks[i] to the ParScanThreadState.  

I think similar things can be done for Parallel GC (and G1).  

Individual GCs can then override PreservedMarksSet::restore_all_marks()  
that does what is currently written as a note in  
ParScanThreadSet::restore_preserved_marks(), depending on collector.  

Btw, I do not think it is useful to write such long notes into the  
code. Further enhancement ideas should be stored in the bug tracker.  
Very very few people are browsing through that code, particular CMS, to  
find work to do :)  

That PreservedMarksSet class, or whatever you want to call it, would  
roughly correspond to G1CollectedHeap::_preserved_objs.  

> A couple of notes:  
>  
> * Please see the note on the JIRA re: why the preserved mark stack on  
> DefNew might still be used when ParNew is used.  

I think above proposal is better, adding a single virtual method to  
some kind of PreservedMarksSet class that can be overridden by the  
collector.  

This keeps this particular evacuation failure functionality private to  
that PreservedMarksSet implementation and not somewhere in the already  
huge CollectedHeap subclasses (for such an "unimportant" feature).  

> * I moved the OopAndMarkOop and OopAndMarkOopStack classes from the  
> G1 file to the shared file so I can re-use them. I thought doing this  
> it’s cleaner than including the G1 file in the shared file.  
>  
> * I’ll re-use the PreservedMarks class for the ParallelGC changes too  
> (see JDK-8146991). In fact, it might be worth also using it in G1  
> too, so that all the logic is in one place. Your call.  

Yes, we should reuse the code everywhere as much as possible (and  
reasonable).  

Other minor comments:  

- the copyright dates are all wrong :), and for the new files, they  
should only contain the year 2016.  

Thanks,  
Thomas  
-----

Tony Printezis | JVM/GC Engineer / VM Team | Twitter

@TonyPrintezis
tprintezis at twitter.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160122/95b0f925/attachment.htm>