8152312: ParNew: Restore preserved marks in parallel
tprintezis at twitter.com
Fri Mar 25 13:35:31 UTC 2016
On March 25, 2016 at 8:56:59 AM, Thomas Schatzl (thomas.schatzl at oracle.com) wrote:
On Thu, 2016-03-24 at 11:36 -0400, Tony Printezis wrote:
> Thanks for the feedback. Updated webrev here:
> - 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.
Some specialized implementation of the method based on the type of E I
First, if it helps you, here are the changes for PS (as applied on top of the ParNew changes):
Note that we now basically duplicate the restore() method,
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).
close to just have two (parallel gc, the rest) implementations of the
class. Let's see your solution.
We have a single method that decides what to do (serial or parallel restoration) and yields to the specific parallel implementation as needed.
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.).
If you don’t like the use of the template here, there’s an alternative. Expose the following:
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. :-)
Either way is totally fine with me. Pick your poison.
Since I do not know the actual implementation of your scheme, but it
would be nice if the code between lines 65 and 68 in
preservedMarks.inline.hpp were moved into a method too, particularly if
the future implementation also has a single-threaded path.
It doesn’t, that loop is not replicated (see the PS webrev).
I would prefer to have the implementation(s) of restore() in the cpp
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*)).
(and possibly hide the one for parallel somewhere very
far away ;) ).
The additional call in this case does not seem to be
problematic vs. performance.
I totally agree with this.
> - 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*);
One comment here: the comment at
70 // Right now, if the executor is not NULL we do the work in
71 // parallel. In the future we might want to do the restoration
72 // serially, if there's only a small number of marks per
Imo is better placed before line 64, cut a lot to something like:
// Decide on the amount of parallelism.
Please add an RFE in the bug tracker explaining the situation about "in
the future" ideas. That would be much better than dumping your thoughts
in some random place in the code.
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...
> the main question will be what policy to use!). I'll do a follow-up
> to this too.
The main goal is not to be perfect imo, but to determine a reasonable
somewhat conservative upper bound on the amount of threads used. Too
many people use too large machines on small problems.
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)
One could argue, that work units < 1ms (or another low value) are not
From that one could determine a "reasonable"
minimum number of references to process per thread, from which you
could then deduct a range of PreservedMarks buffers that each thread
could use (assuming that the actual references are evenly distributed
across these sets of references).
At least that's what I would start with.
> - 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.
Another reason is that nowadays practically everything done during a
STW pause must somehow be implemented to run in parallel to be actually
useful (except for serial gc obviously). So this marker is kind of
misleading and redundant.
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).
Not really insisting on this, but it may be that some future general cleanup will modify this (nothing actually planned from me).
Tony Printezis | JVM/GC Engineer / VM Team | Twitter
tprintezis at twitter.com
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev