<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;">Inline.</div> <br><p class="airmail_on">On March 25, 2016 at 8:56:59 AM, Thomas Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>) wrote:</p> <div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div></div><div>Hi,<span class="Apple-converted-space"> </span><br><br>On Thu, 2016-03-24 at 11:36 -0400, Tony Printezis wrote:<span class="Apple-converted-space"> </span><br>> Thomas,<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> Thanks for the feedback. Updated webrev here:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> http://cr.openjdk.java.net/~tonyp/8152312/webrev.1/<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> Changes:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> - Your main comment was whether to make the serial vs. parallel<span class="Apple-converted-space"> </span><br>> decision in DefNew or in restore(). I liked the suggestion of either<span class="Apple-converted-space"> </span><br>> passing NULL or the workers to restore(WorkGang*) and deciding in<span class="Apple-converted-space"> </span><br>> restore(…) what to do. The only problem is that ParallelGC doesn’t<span class="Apple-converted-space"> </span><br>> use a WorkGang, it uses a GCTaskManager instead.<span class="Apple-converted-space"> </span><br><br>Drat!<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Indeed. :-)</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>> And anything we do in restore(...) would be nice to also take<span class="Apple-converted-space"> </span><br>> advantage of in ParallelGC. So, I came up with a cunning scheme to do<span class="Apple-converted-space"> </span><br>> that. Let me know what you think. :-) Yes, it works fine with<span class="Apple-converted-space"> </span><br>> ParallelGC too (I already implemented/tested the changes, I’ll open<span class="Apple-converted-space"> </span><br>> them for code review once this is done.) I left a restore() method<span class="Apple-converted-space"> </span><br>> there temporarily to be used by ParallelGC but it will go away.<span class="Apple-converted-space"> </span><br><br>Some specialized implementation of the method based on the type of E I<span class="Apple-converted-space"> </span><br>guess.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>First, if it helps you, here are the changes for PS (as applied on top of the ParNew changes):</p><p>http://cr.openjdk.java.net/~tonyp/8152312/webrev.1.ps/</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>Note that we now basically duplicate the restore() method,<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Depends which restore() method you’re talking about. restore(E*) remains unchanged in the PS changes. We have to introduce a new restore_internal() that knows how to use the GCTaskManager (and, yes, there’s a bit of code replication in that task; but only a few lines). So, in the future, if we add some additional logic to decide whether to do the restoration serially or in parallel, we’ll only have to change restore(E*) and the “executor”-specific code remains the same (I have hacked up a version of that too, as a proof-of-concept, and it does work out fine, FWIW).</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div>it gets<span class="Apple-converted-space"> </span><br>close to just have two (parallel gc, the rest) implementations of the<span class="Apple-converted-space"> </span><br>class. Let's see your solution.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>We have a single method that decides what to do (serial or parallel restoration) and yields to the specific parallel implementation as needed. </p><p>I used a template so that I can put the shared implementation in one place and still be able to easily call the “executor”-specific methods when needed without having lots of ridiculous boilerplate code (pass both executors to the method, make sure only one of them is non-NULL, based on which one is not NULL call the appropriate parallel method, etc.).</p><p>If you don’t like the use of the template here, there’s an alternative. Expose the following:</p><p>restore(WorkGang* workers)</p><p>restore(GCTaskManager* gc_task_manager)</p><p>and each if those first calls a private restore_internal() which either does the restoration serially or decides not to and tells the “executor”-specific restore() that it needs to do the work in parallel. But you said in your e-mail that you wanted a single restore() which yields to the parallel when needed, which is what I tried to do in the webrev. :-)</p><p>Either way is totally fine with me. Pick your poison.</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>Since I do not know the actual implementation of your scheme, but it<span class="Apple-converted-space"> </span><br>would be nice if the code between lines 65 and 68 in<span class="Apple-converted-space"> </span><br>preservedMarks.inline.hpp were moved into a method too, particularly if<span class="Apple-converted-space"> </span><br>the future implementation also has a single-threaded path.<span class="Apple-converted-space"> </span><span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>It doesn’t, that loop is not replicated (see the PS webrev).</p><p><br></p><div><div><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>I would prefer to have the implementation(s) of restore() in the cpp<span class="Apple-converted-space"> </span><br>file(s) though</div></div></span></blockquote></div><p><br></p></div><p>I agree. But, given that restore(E*) is a template, I assume I have to put it in the .inline.hpp file. If you really want to in the .cpp file, I think we have to go with the scheme I describe above (expose restore(WorkGang*) and restore(GCTaskManager*)).</p><p><br></p><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><span class="Apple-converted-space"> </span>(and possibly hide the one for parallel somewhere very<span class="Apple-converted-space"> </span><br>far away ;) ).<span class="Apple-converted-space"> </span></div></div></span></blockquote></div></div><p><br></p><p>:-)</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div>The additional call in this case does not seem to be<span class="Apple-converted-space"> </span><br>problematic vs. performance.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I totally agree with this.</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>> - Yes, if each stack has a small number of entries we’re better off<span class="Apple-converted-space"> </span><br>> doing the restoration serially instead of firing up the workers. This<span class="Apple-converted-space"> </span><br>> will be a small localized change to what I have in the webrev (we’ll<span class="Apple-converted-space"> </span><br>> only have to change restore(E*);<span class="Apple-converted-space"> </span><br><br>One comment here: the comment at<span class="Apple-converted-space"> </span><br><br>70 // Right now, if the executor is not NULL we do the work in<span class="Apple-converted-space"> </span><br>71 // parallel. In the future we might want to do the restoration<span class="Apple-converted-space"> </span><br>72 // serially, if there's only a small number of marks per<span class="Apple-converted-space"> </span><br>stack.<span class="Apple-converted-space"> </span><br><br>Imo is better placed before line 64, cut a lot to something like:<span class="Apple-converted-space"> </span><br><br>// Decide on the amount of parallelism.<span class="Apple-converted-space"> </span><br><br>Please add an RFE in the bug tracker explaining the situation about "in<span class="Apple-converted-space"> </span><br>the future" ideas. That would be much better than dumping your thoughts<span class="Apple-converted-space"> </span><br>in some random place in the code.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I put the comment when we were calling the parallel version, so I didn’t think it was a random place. :-) But, sure, I’ll move it earlier...</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>> the main question will be what policy to use!). I'll do a follow-up<span class="Apple-converted-space"> </span><br>> to this too.<span class="Apple-converted-space"> </span><br><br>The main goal is not to be perfect imo, but to determine a reasonable<span class="Apple-converted-space"> </span><br>somewhat conservative upper bound on the amount of threads used. Too<span class="Apple-converted-space"> </span><br>many people use too large machines on small problems.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I agree, I don’t think it needs to be perfect and we should spend too much time on it. I was actually going to propose a very simple scheme that makes the determination based on the size of the stacks. I.e., start iterating over the stacks and, for every stack we check its size. If it's under a limit we just go ahead and do the work serially. Eventually, if we come across a stack whose size is “too large” we bail out of the serial phase and yield of the parallel version (and we can notify the parallel version that we’ve already done N stacks and it should ignore them). Even if there’s a small number of GC threads (2 / 4), it’s still worth doing the work serially if only a small number of marks have been preserved. (IMHO)</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>One could argue, that work units < 1ms (or another low value) are not<span class="Apple-converted-space"> </span><br>worth distributing.</div></div></span></blockquote></div><p><br></p><p>I agree.</p><p><br></p><div><div><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><span class="Apple-converted-space"> </span>From that one could determine a "reasonable"<span class="Apple-converted-space"> </span><br>minimum number of references to process per thread, from which you<span class="Apple-converted-space"> </span><br>could then deduct a range of PreservedMarks buffers that each thread<span class="Apple-converted-space"> </span><br>could use (assuming that the actual references are evenly distributed<span class="Apple-converted-space"> </span><br>across these sets of references).<span class="Apple-converted-space"> </span><br><br>At least that's what I would start with.<span class="Apple-converted-space"> </span><br><br>> - Ah, yes, I’m glad there’s an atomic add for size_t in JDK 9. :-)<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> - Re: ParRestoreTask vs. RestoreTask : I tend to add “Par” as the<span class="Apple-converted-space"> </span><br>> tasks should be safe to use in parallel (even if in practice they are<span class="Apple-converted-space"> </span><br>> not). Hope that’s OK.<span class="Apple-converted-space"> </span><br><br>Another reason is that nowadays practically everything done during a<span class="Apple-converted-space"> </span><br>STW pause must somehow be implemented to run in parallel to be actually<span class="Apple-converted-space"> </span><br>useful (except for serial gc obviously). So this marker is kind of<span class="Apple-converted-space"> </span><br>misleading and redundant.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Depends whether you see “Par” as “it will be done in parallel” or “it might be done in parallel”. I see it as the latter (and there’s lots of cases in the code where Par is used for such classes). </p><p><br></p><p>Tony</p><p><br></p><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>Not really insisting on this, but it may be that some future general cleanup will modify this (nothing actually planned from me).<span class="Apple-converted-space"> </span></div></div></span></blockquote></div></div><p><br></p><div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span><div><div><br>Thanks,<span class="Apple-converted-space"> </span><br>Thomas<span class="Apple-converted-space"> </span><br><br></div></div></span></blockquote><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div> <div id="bloop_sign_1458911515342786048" 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>