8152312: ParNew: Restore preserved marks in parallel

Thomas Schatzl thomas.schatzl at oracle.com
Tue Mar 22 10:49:15 UTC 2016


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



More information about the hotspot-gc-dev mailing list