[PATCH] enhancement to ParallelScavenge Full GC

ray alex sky1young at gmail.com
Thu Dec 24 15:15:23 UTC 2015


Hi Mikael, Thomas,

According to your recommendation,

I have put all the cache-related variables in the ParCompactionManager class.

Now that we do not need to modify the Thread class anymore.

May I have your reviews on this new patch?



2015-12-21 23:34 GMT+08:00 Mikael Gerdin <mikael.gerdin at oracle.com>:

> 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
>>
>>
>


-- 
Lei Tianyang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20151224/16ce6211/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parallelScavenge_v3.patch
Type: text/x-patch
Size: 25507 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20151224/16ce6211/parallelScavenge_v3.patch>


More information about the hotspot-gc-dev mailing list