8152312: ParNew: Restore preserved marks in parallel

[Tony Printezis]

Thomas,

Thanks for the feedback. Updated webrev here:

http://cr.openjdk.java.net/~tonyp/8152312/webrev.1/

Changes:

- Your main comment was whether to make the serial vs. parallel decision in DefNew or in restore(). I liked the suggestion of either passing NULL or the workers to restore(WorkGang*) and deciding in restore(…) what to do. The only problem is that ParallelGC doesn’t use a WorkGang, it uses a GCTaskManager instead. And anything we do in restore(...) would be nice to also take advantage of in ParallelGC. So, I came up with a cunning scheme to do that. Let me know what you think. :-) Yes, it works fine with ParallelGC too (I already implemented/tested the changes, I’ll open them for code review once this is done.) I left a restore() method there temporarily to be used by ParallelGC but it will go away…

- Yes, if each stack has a small number of entries we’re better off doing the restoration serially instead of firing up the workers. This will be a small localized change to what I have in the webrev (we’ll only have to change restore(E*); the main question will be what policy to use!). I'll do a follow-up to this too.

- Ah, yes, I’m glad there’s an atomic add for size_t in JDK 9. :-)

- Re: ParRestoreTask vs. RestoreTask : I tend to add “Par” as the tasks should be safe to use in parallel (even if in practice they are not). Hope that’s OK.

Tony

On March 22, 2016 at 6:49:24 AM, Thomas Schatzl (thomas.schatzl at oracle.com) wrote:

Hi tony,  

On Mon, 2016-03-21 at 11:58 -0400, Tony Printezis wrote:  
> Hi all,  
>  
> I was advised to get the ParNew changes reviewed separately from the  
> G1 changes (see 8151556). Here are just the PreservedMarks* changes +  
> the ParNew change:  
>  
> http://cr.openjdk.java.net/~tonyp/8152312/webrev.0/  

Thanks for this change.  

> I’ll redo the G1 changes for 8151556 to depend on this change.  
>  
> I also piggy-backed the following change (for DefNew):  
>  
> log_debug(gc)("Promotion failed”);  
> ->  
> log_info(gc, promotion)("Promotion failed”);  
>  
> so that it’s consistent with what ParNew does. Any objections?  

No :)  

Some comments however:  

- I would prefer if the decision to do parallel/serial mark restoration  
were hidden in PreservedMarksSet.  

It simplifies the use of the class (no need to even think about whether  
the correct method is called).  

Just pass the WorkGang to the restore() method. There still can be  
separate, private parallel/serial mark restoration methods.  

Further, arbitrary decisions on when to use what method can be made  
(e.g. because I am pretty sure in many cases using all worker threads  
is not a good idea, e.g. for almost or completely empty lists) without  
the need to change anything in the callers.  

Additionally the assert_empty and the log message do not need to be  
duplicated at the end of restore and par_restore.  

- please use size_t for ParRestoreTask::_total_size. There is an  
Atomic::add() for size_t.  

- preservedMarks.cpp:95: the comment is a proper sentence, please start  
with upper case and end with full stop.  

- and finally the bike-shedding topic (feel free to ignore), I would  
prefer to not use the "Par" prefix to the ParRestoreTask task. It's  
kind of redundant because these tasks should be kind of agnostic in how  
many workers are used.  

:)  

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/20160324/650b7498/attachment.htm>