[PATCH] enhancement to ParallelScavenge Full GC

[Mikael Gerdin]

Hi,

On 2015-12-21 16:09, ray alex wrote:
> Hi Thomas, Mikael,
>
> I've refined the code based on your recommendations on the code style.
>
> However, in my opinion, to provide an additional context in GCThread instead of Thread may lead to massive modifications
>
> and I'm not sure if it may bring some impact on the correctness of other codes.
> So I have not revised this part so far, in order to keep our patch pure and independent.
> (Or maybe it's because I did not quite catch your ideas?)
>
> May I have your reviews on the new patch?

It may be that your patch works, but one of the issues with working with 
such a large code base as this is to not just make changes which are 
functionally correct, but they also need to "fit in" and make sense with 
how the rest of the code works.

Putting 3 member variables in the Thread superclass just because a small 
phase of one of the garbage collectors needs some thread local state 
does not "fit in".

In HotSpot, all the garbage collectors maintain their thread-local state 
by passing explicit context parameters.
I think that the only solution which makes sense for the fix to 
ParallelCompact is to modify
ParCompactionManager::update_contents(oop obj)
to pass itself like this:
obj->pc_update_contents(this);

and modifying
oopDesc::pc_update_contents(ParCompactionManager* cm)
to pass it on:
k->oop_pc_update_pointers(this, cm);

and modify all Klass subclasses to use the new parameter such as

void InstanceKlass::oop_pc_update_pointers(oop obj, 
ParCompactionManager* cm) {
   PSParallelCompact::AdjustPointerClosre closure(cm);
   oop_oop_iterate_oop_maps<true>(obj, &cm);
}

That way the call
     oop new_obj = (oop)summary_data().calc_new_pointer(obj);
in AdjustPointerClosure
can pass the "cm" parameter on to live_words_in_range.

/Mikael


>
>
>
> 2015-12-16 0:15 GMT+08:00 Mikael Gerdin <mikael.gerdin at oracle.com
> <mailto:mikael.gerdin at oracle.com>>:
>
>     Hi,
>
>
>     On 2015-12-14 17:49, Thomas Schatzl wrote:
>
>         Hi,
>
>         On Mon, 2015-12-14 at 23:57 +0800, ray alex wrote:
>
>             Thanks, Thomas!
>
>             I will refine my code carefully according to your
>             suggestions as soon
>             as possible.
>
>
>         Thanks.
>
>
>             Since I'm new in contributing to OpenJDK, actually I'm not
>             sure how to
>             upload when my new code is ready?
>                Just upload an attach of diffs as I did, or some other ways?
>
>
>         The best way as long as we have no confirmation about the OCA
>         signing,
>         is to attach a patch (or a webrev, see
>         http://openjdk.java.net/contribute/) and send it to this list.
>         Later it
>         will be more convenient to send me a patch that I will convert
>         into a
>         webrev and put on cr.openjdk.java.net <http://cr.openjdk.java.net>.
>
>         As soon as you have two contributions, you can upload patches
>         yourselves
>         onto your own account on cr.openjdk.java.net
>         <http://cr.openjdk.java.net>.
>
>
>             Besides, I'm currently processing my signed OCA for the
>             committer
>             rights,
>             I'm not sure how long it will take, does it matter?
>
>
>         The OCA is only for us to be able to accept your patch in
>         Hotspot. The
>         various levels of contributor status are a different matter, and
>         signify
>         your level of expertise within the community.
>
>         See also http://openjdk.java.net/contribute/ for more information.
>
>             As for the VM Thread, it does a serial adjust_roots() within the
>             current thread (VM thread) during
>             PSParallelCompact::invoke_no_policy().
>                I think psCompactionManager.cpp:117 and the surrounding
>             comments
>             confirm the use of VMThread in full GC.
>             Do you agree with my above understanding of the use of VMThread?
>
>         You are right :(
>
>         There are multiple options to provide an additional per-thread
>         context:
>
>         - passing around some context. Looks like lots of work.
>
>
>     I actually think this is the correct way to go.
>     In my mind you would only have to change
>     ParCompactionMangager::update_contents to pass on the
>     ParCompactionManager* through oopDesc::pc_update_contents to
>     *Klass::oop_pc_update_contents
>     It's similar to how the marking phase passes around the current
>     worker's compaction manager.
>
>     Then the AdjustPointerClosure needs to become a
>     per-ParCompactionManager thing and then it's all set :)
>
>     /Mikael
>
>
>
>         - change the code so that even that serial portion is performed
>         by some
>         GCTaskThread (which actually sounds best to me)
>         - Another would be to add a field to both GCTaskThread and
>         VMThread, and
>         via a virtual method in Thread get the necessary values.
>         - make VMThread a WorkerThread (that has an id that can be used
>         to query
>         an extra data structure for this cache)
>         - others
>
>         Let's see if other people have a better idea.
>
>         Thanks,
>             Thomas
>
>
>
>
>
>
>
> --
> Lei Tianyang
>