[PATCH] enhancement to ParallelScavenge Full GC
ray alex
sky1young at gmail.com
Sun Jan 10 14:16:18 UTC 2016
Hi,
By following the conventions, I remove the prefix "get_" and rename
getter/setter to "last_query_*" in this patch version 4 (attached).
Besides, I ran the measurements mentioned before (both for version 3 and
recheck this version 4 ), and there was no regressions.
May I have your reviews on this patch?
Thanks!
2016-01-07 22:15 GMT+08:00 Mikael Gerdin <mikael.gerdin at oracle.com>:
> Hi,
>
>
> On 2015-12-24 16:15, ray alex wrote:
>
>> 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.
>>
>
> Nice! This patch has a much nicer structure.
> There are some naming conventions that the patch does not follow:
>
> * we don't prefix getters with get_
> * we rarely use short hands or initialisms (such as "lwir_beg")
>
> Did you re-run your performance measurements on version 3 of the patch to
> ensure that the refactorings I suggested don't introduce any regressions?
>
> /Mikael
>
>
>
>> May I have your reviews on this new patch?
>>
>>
>>
>> 2015-12-21 23:34 GMT+08:00 Mikael Gerdin <mikael.gerdin at oracle.com
>> <mailto: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>
>> <mailto: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> <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>
>> <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
>>
>>
>
--
Lei Tianyang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160110/3d9359dc/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parallelScavenge_v4.patch
Type: text/x-patch
Size: 25538 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160110/3d9359dc/parallelScavenge_v4.patch>
More information about the hotspot-gc-dev
mailing list