<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;">Thanks for the feedback. Updated webrev here:</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;"><a href="http://cr.openjdk.java.net/~tonyp/8152312/webrev.1/">http://cr.openjdk.java.net/~tonyp/8152312/webrev.1/</a></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;">Changes:</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;">- 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…</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;">- 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.</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;">- Ah, yes, I’m glad there’s an atomic add for size_t in JDK 9. :-)</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;">- 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.</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;">Tony</div> <br><p class="airmail_on">On March 22, 2016 at 6:49:24 AM, Thomas Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>) wrote:</p> <blockquote type="cite" class="clean_bq"><span><div><div></div><div>Hi tony,

<br>

<br>On Mon, 2016-03-21 at 11:58 -0400, Tony Printezis wrote:

<br>> Hi all,

<br>>  

<br>> I was advised to get the ParNew changes reviewed separately from the

<br>> G1 changes (see 8151556). Here are just the PreservedMarks* changes +

<br>> the ParNew change:

<br>>  

<br>> http://cr.openjdk.java.net/~tonyp/8152312/webrev.0/

<br>

Thanks for this change.

<br>

> I’ll redo the G1 changes for 8151556 to depend on this change.

<br>>  

<br>> I also piggy-backed the following change (for DefNew):

<br>>  

<br>> log_debug(gc)("Promotion failed”);

<br>> ->

<br>> log_info(gc, promotion)("Promotion failed”);

<br>>  

> so that it’s consistent with what ParNew does. Any objections?

<br>

<br>No :)

<br>

<br>Some comments however:

<br>

<br>- I would prefer if the decision to do parallel/serial mark restoration

were hidden in PreservedMarksSet.

<br>

<br>It simplifies the use of the class (no need to even think about whether

<br>the correct method is called).  

<br>

<br>Just pass the WorkGang to the restore() method. There still can be

separate, private parallel/serial mark restoration methods.

<br>

<br>Further, arbitrary decisions on when to use what method can be made

<br>(e.g. because I am pretty sure in many cases using all worker threads

<br>is not a good idea, e.g. for almost or completely empty lists) without

the need to change anything in the callers.

<br>

<br>Additionally the assert_empty and the log message do not need to be

duplicated at the end of restore and par_restore.

<br>

<br>- please use size_t for ParRestoreTask::_total_size. There is an

Atomic::add() for size_t.

<br>

<br>- preservedMarks.cpp:95: the comment is a proper sentence, please start

with upper case and end with full stop.

<br>

<br>- and finally the bike-shedding topic (feel free to ignore), I would

<br>prefer to not use the "Par" prefix to the ParRestoreTask task. It's

<br>kind of redundant because these tasks should be kind of agnostic in how

many workers are used.

<br>

<br>:)

<br>

<br>Thanks,

<br>  Thomas

<br>

<br></div></div></span></blockquote> <div id="bloop_sign_1458832651320966912" 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>