[PATCH] enhancement to ParallelScavenge Full GC
Mikael Gerdin
mikael.gerdin at oracle.com
Thu Jan 7 14:15:26 UTC 2016
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
>
More information about the hotspot-gc-dev
mailing list