<div dir="ltr"><div>Hi, </div><div>By following the conventions, I remove the prefix "get_" and rename getter/setter to "last_query_*" in this patch version 4 (attached).</div><div>Besides, I ran the measurements mentioned before (both for version 3 and recheck this version 4 ), and there was no regressions.</div><div><br></div><div>May I have your reviews on this patch?</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks!</div><div class="gmail_extra"><br><div class="gmail_quote">2016-01-07 22:15 GMT+08:00 Mikael Gerdin <span dir="ltr"><<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<span class=""><br>
<br>
<br>
On 2015-12-24 16:15, ray alex wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Mikael, Thomas,<br>
<br>
According to your recommendation,<br>
<br>
I have put all the cache-related variables in the ParCompactionManager class.<br>
<br>
Now that we do not need to modify the Thread class anymore.<br>
</blockquote>
<br></span>
Nice! This patch has a much nicer structure.<br>
There are some naming conventions that the patch does not follow:<br>
<br>
* we don't prefix getters with get_<br>
* we rarely use short hands or initialisms (such as "lwir_beg")<br>
<br>
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?<br>
<br>
/Mikael<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
May I have your reviews on this new patch?<br>
<br>
<br>
<br>
2015-12-21 23:34 GMT+08:00 Mikael Gerdin <<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a><br></span>
<mailto:<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a>>>:<div><div class="h5"><br>
<br>
Hi,<br>
<br>
On 2015-12-21 16:09, ray alex wrote:<br>
<br>
Hi Thomas, Mikael,<br>
<br>
I've refined the code based on your recommendations on the code<br>
style.<br>
<br>
However, in my opinion, to provide an additional context in<br>
GCThread instead of Thread may lead to massive modifications<br>
<br>
and I'm not sure if it may bring some impact on the correctness<br>
of other codes.<br>
So I have not revised this part so far, in order to keep our<br>
patch pure and independent.<br>
(Or maybe it's because I did not quite catch your ideas?)<br>
<br>
May I have your reviews on the new patch?<br>
<br>
<br>
It may be that your patch works, but one of the issues with working<br>
with such a large code base as this is to not just make changes<br>
which are functionally correct, but they also need to "fit in" and<br>
make sense with how the rest of the code works.<br>
<br>
Putting 3 member variables in the Thread superclass just because a<br>
small phase of one of the garbage collectors needs some thread local<br>
state does not "fit in".<br>
<br>
In HotSpot, all the garbage collectors maintain their thread-local<br>
state by passing explicit context parameters.<br>
I think that the only solution which makes sense for the fix to<br>
ParallelCompact is to modify<br>
ParCompactionManager::update_contents(oop obj)<br>
to pass itself like this:<br>
obj->pc_update_contents(this);<br>
<br>
and modifying<br>
oopDesc::pc_update_contents(ParCompactionManager* cm)<br>
to pass it on:<br>
k->oop_pc_update_pointers(this, cm);<br>
<br>
and modify all Klass subclasses to use the new parameter such as<br>
<br>
void InstanceKlass::oop_pc_update_pointers(oop obj,<br>
ParCompactionManager* cm) {<br>
PSParallelCompact::AdjustPointerClosre closure(cm);<br>
oop_oop_iterate_oop_maps<true>(obj, &cm);<br>
}<br>
<br>
That way the call<br>
oop new_obj = (oop)summary_data().calc_new_pointer(obj);<br>
in AdjustPointerClosure<br>
can pass the "cm" parameter on to live_words_in_range.<br>
<br>
/Mikael<br>
<br>
<br>
<br>
<br>
<br>
2015-12-16 0:15 GMT+08:00 Mikael Gerdin<br>
<<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a> <mailto:<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a>><br></div></div>
<mailto:<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a><div><div class="h5"><br>
<mailto:<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a>>>>:<br>
<br>
Hi,<br>
<br>
<br>
On 2015-12-14 17:49, Thomas Schatzl wrote:<br>
<br>
Hi,<br>
<br>
On Mon, 2015-12-14 at 23:57 +0800, ray alex wrote:<br>
<br>
Thanks, Thomas!<br>
<br>
I will refine my code carefully according to your<br>
suggestions as soon<br>
as possible.<br>
<br>
<br>
Thanks.<br>
<br>
<br>
Since I'm new in contributing to OpenJDK, actually<br>
I'm not<br>
sure how to<br>
upload when my new code is ready?<br>
Just upload an attach of diffs as I did, or some<br>
other ways?<br>
<br>
<br>
The best way as long as we have no confirmation about<br>
the OCA<br>
signing,<br>
is to attach a patch (or a webrev, see<br>
<a href="http://openjdk.java.net/contribute/" rel="noreferrer" target="_blank">http://openjdk.java.net/contribute/</a>) and send it to this list.<br>
Later it<br>
will be more convenient to send me a patch that I will<br>
convert<br>
into a<br>
webrev and put on <a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">cr.openjdk.java.net</a><br></div></div>
<<a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">http://cr.openjdk.java.net</a>> <<a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">http://cr.openjdk.java.net</a>>.<div><div class="h5"><br>
<br>
As soon as you have two contributions, you can upload<br>
patches<br>
yourselves<br>
onto your own account on <a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">cr.openjdk.java.net</a><br>
<<a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">http://cr.openjdk.java.net</a>><br>
<<a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">http://cr.openjdk.java.net</a>>.<br>
<br>
<br>
<br>
Besides, I'm currently processing my signed OCA for the<br>
committer<br>
rights,<br>
I'm not sure how long it will take, does it matter?<br>
<br>
<br>
The OCA is only for us to be able to accept your patch in<br>
Hotspot. The<br>
various levels of contributor status are a different<br>
matter, and<br>
signify<br>
your level of expertise within the community.<br>
<br>
See also <a href="http://openjdk.java.net/contribute/" rel="noreferrer" target="_blank">http://openjdk.java.net/contribute/</a> for more<br>
information.<br>
<br>
As for the VM Thread, it does a serial<br>
adjust_roots() within the<br>
current thread (VM thread) during<br>
PSParallelCompact::invoke_no_policy().<br>
I think psCompactionManager.cpp:117 and the<br>
surrounding<br>
comments<br>
confirm the use of VMThread in full GC.<br>
Do you agree with my above understanding of the use<br>
of VMThread?<br>
<br>
You are right :(<br>
<br>
There are multiple options to provide an additional<br>
per-thread<br>
context:<br>
<br>
- passing around some context. Looks like lots of work.<br>
<br>
<br>
I actually think this is the correct way to go.<br>
In my mind you would only have to change<br>
ParCompactionMangager::update_contents to pass on the<br>
ParCompactionManager* through oopDesc::pc_update_contents to<br>
*Klass::oop_pc_update_contents<br>
It's similar to how the marking phase passes around the current<br>
worker's compaction manager.<br>
<br>
Then the AdjustPointerClosure needs to become a<br>
per-ParCompactionManager thing and then it's all set :)<br>
<br>
/Mikael<br>
<br>
<br>
<br>
- change the code so that even that serial portion is<br>
performed<br>
by some<br>
GCTaskThread (which actually sounds best to me)<br>
- Another would be to add a field to both GCTaskThread and<br>
VMThread, and<br>
via a virtual method in Thread get the necessary values.<br>
- make VMThread a WorkerThread (that has an id that can<br>
be used<br>
to query<br>
an extra data structure for this cache)<br>
- others<br>
<br>
Let's see if other people have a better idea.<br>
<br>
Thanks,<br>
Thomas<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
--<br>
Lei Tianyang<br>
<br>
<br>
<br>
<br>
<br>
--<br>
Lei Tianyang<br>
<br>
</div></div></blockquote>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Lei Tianyang<div><br></div></div>
</div></div>