<div dir="ltr"><pre class="" style="margin-top:0px;margin-bottom:0px;font-family:inherit;font-size:14px;white-space:pre-wrap;color:rgb(0,0,0);line-height:22.4px">Hi Mikael, Thomas,<br><br>According to your recommendation, </pre><pre class="" style="margin-top:0px;margin-bottom:0px;font-family:inherit;font-size:14px;white-space:pre-wrap;color:rgb(0,0,0);line-height:22.4px">I have put all the cache-related variables in the ParCompactionManager class. </pre><pre class="" style="margin-top:0px;margin-bottom:0px;font-family:inherit;font-size:14px;white-space:pre-wrap;color:rgb(0,0,0);line-height:22.4px">Now that we do not need to modify the Thread class anymore.<br><br>May I have your reviews on this new patch?</pre><pre class="" style="margin-top:0px;margin-bottom:0px;font-family:inherit;font-size:14px;white-space:pre-wrap;color:rgb(0,0,0);line-height:22.4px"><br></pre></div><div class="gmail_extra"><br><div class="gmail_quote">2015-12-21 23:34 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>
On 2015-12-21 16:09, ray alex wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Thomas, Mikael,<br>
<br>
I've refined the code based on your recommendations on the code style.<br>
<br>
However, in my opinion, to provide an additional context in 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 of other codes.<br>
So I have not revised this part so far, in order to keep our 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>
</blockquote>
<br></span>
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.<br>
<br>
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".<br>
<br>
In HotSpot, all the garbage collectors maintain their thread-local state by passing explicit context parameters.<br>
I think that the only solution which makes sense for the fix to 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, 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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<br>
<br>
2015-12-16 0:15 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>>>:<span class=""><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 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 other ways?<br>
<br>
<br>
The best way as long as we have no confirmation about 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 convert<br>
into a<br></span>
webrev and put on <a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">cr.openjdk.java.net</a> <<a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">http://cr.openjdk.java.net</a>>.<span class=""><br>
<br>
As soon as you have two contributions, you can upload 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></span>
<<a href="http://cr.openjdk.java.net" rel="noreferrer" target="_blank">http://cr.openjdk.java.net</a>>.<div><div class="h5"><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 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 information.<br>
<br>
As for the VM Thread, it does a serial adjust_roots() within the<br>
current thread (VM thread) during<br>
PSParallelCompact::invoke_no_policy().<br>
I think psCompactionManager.cpp:117 and the surrounding<br>
comments<br>
confirm the use of VMThread in full GC.<br>
Do you agree with my above understanding of the use of VMThread?<br>
<br>
You are right :(<br>
<br>
There are multiple options to provide an additional 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 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 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>
</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>