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

[Thomas Schatzl]

Hi Tony,

On Fri, 2016-01-22 at 16:59 -0200, Tony Printezis wrote:
> 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.

No problem.

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

Not sure if this exactly corresponds to my understanding of the
lifetime of ParScanThreadState (PSS in the following): to me it is a
helper data structure that is for storing information that comes up
during the evacuation phase only.
Again to me, preserved mark handling/restoration, also because it needs
to be done in a separate parallel phase after main evacuation, has a
different lifetime and so its owner should not be the PSS.

The other thing is, the threads restoring the marks may be different
ones than the ones pushing marks onto them. I.e. use a different amount
of threads for mark restoration than evacuation to better balance the
(available) workload. Then having the PSS the "owner" of the stacks
just gets in the way.

[At least for G1 we might look into that, i.e. using different amount
of threads for different parallel phases for various reasons quite
soon. Which means, please keep the preserved mark stacks separate from
its PSS in at least G1 :]

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

The main idea of this unified marks restoration method would be that it

- it is known where the mark restoration code is located. (exactly in
 PreservedMarksSet::restore_marks()), and not in more or less randomly
named methods depending on collector.

- makes the main evacuation method for all collectors a bit more
uniform. I.e. all do

evacuate_collection_set();
if (evacuation_failed()) {
  PreservedMarksSet::restore_marks()
}

- puts mark restoration code in subclasses (searching for subclasses of
PreservedMarksStack is much easier for me than guessing the name of the
day for a particular collector) instead of putting it somewhere in
CollectedHeap instances. You could then also remove some mark
restoration helper types out of the global namespace.

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

I am aware that the actual code for restoring the marks can not be
shared. I see it as a way to improve readability of the code.

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

I do not insist implementing this the way I would do that. That idea
may be done at a different time, or altogether scrapped.

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

Re-reading it again, it's fine with me to keep that note after all. I
seem to have misunderstood the purpose of this wall of text :)

> I’ll fix the copyright dates.

Thanks,
  Thomas