[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.java.net/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.java.net/pipermail/hotspot-gc-dev/attachments/20160110/3d9359dc/parallelScavenge_v4.patch>


More information about the hotspot-gc-dev mailing list