[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.org/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.org/pipermail/hotspot-gc-dev/attachments/20151224/16ce6211/parallelScavenge_v3.patch>
More information about the hotspot-gc-dev
mailing list