<p dir="ltr">Hi Albert,</p>
<p dir="ltr">Small and possibly useless suggestion, but, if you decide to keep printing warning message every X sweeps, maybe use a power of 2 interval rather than 10 (e.g. 16 is just as good probably)? That saves the possibly expensive % operation and can be turned into an & instead (I.e. counter & (INTERVAL - 1) == 0). Maybe overkill, so ignore this :).</p>
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Nov 8, 2013 10:33 AM, "Albert Noll" <<a href="mailto:albert.noll@oracle.com">albert.noll@oracle.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Vladimir,<br>
<br>
thanks for looking at the patch. I hope that this version addresses all issues that<br>
you brought up. Here is a high-level overview of the changes since the last version:<br>
<br>
* We keep track of code cache state changes that also happen outside the sweeper.<br>
I re-installed notify, which is now called report_state_change() and is doing the job.<br>
report_state_change() checks if enough state has changed and enables the sweeper<br>
(it sets _should_sweep) to true. We reset _bytes_changed only once we have finished<br>
a while sweep cycle. That seems to make sense to me.<br>
<br>
* I added code that prints out every 10th warning that the compiler has been disabled.<br>
I also added a warning that compilation has been enabled again. I think if we print a message<br>
that compilation has been disabled, we should also print a message (maybe not a warning)<br>
that was enabled again.<br>
<br>
Here is the new webrev:<br>
<a href="http://cr.openjdk.java.net/~anoll/8027593/webrev.02/" target="_blank">http://cr.openjdk.java.net/~<u></u>anoll/8027593/webrev.02/</a><br>
<br>
Best,<br>
Albert<br>
<br>
On 11/07/2013 11:04 PM, Vladimir Kozlov wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 11/7/13 1:36 PM, Vladimir Kozlov wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Nice work, Albert<br>
<br>
One concern is transition "alive -> not_entrant" is counted only when<br>
the nmethod needs to be flushed because you removed in notify() in<br>
nmethod::make_not_entrant_or_<u></u>zombie(). Also make_zombie() is called from<br>
other places.<br>
I think _bytes_changed should be updated by NMethodSweeper::notify() so<br>
don't remove it from nmethod.cpp. _bytes_changed should be reset when we<br>
finished sweep instead of before each sweep.<br>
</blockquote>
<br>
May be reset in both places actually. One to check that there were updates between sweeps and an other during sweep (as you do currently).<br>
<br>
Thanks,<br>
Vladimir<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Can you keep comments in code which initialize static variables (first<br>
changes in sweeper.cpp)?<br>
<br>
Please narrow your main comment, chars 90 chars per line.<br>
<br>
What is the second place? (initialization should not be count):<br>
<br>
+ // of the two places where should_sweep can be set to true.<br>
<br>
You need to clear state in the comment conditions when we sweep. Like<br>
you did in the replay:<br>
<br>
(1) The code cache is getting full<br>
(2) There are sufficient state changes in the last sweep.<br>
(3) We have not been sweeping for 'some time'<br>
<br>
Split into 2 lines:<br>
<br>
+ int wait_until_next_sweep = (ReservedCodeCacheSize / (16 * M)) -<br>
time_since_last_sweep - CodeCache::reverse_free_ratio(<u></u>);<br>
<br>
In the print format do not use %p, use PTR_FORMAT for 'nm'.<br>
<br>
Thanks,<br>
Vladimir<br>
<br>
On 11/7/13 3:27 AM, Albert Noll wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The previous mail contains an error. See inline.<br>
<br>
Albert<br>
<br>
<br>
On 11/07/2013 12:20 PM, Albert Noll wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Vladimir, Igor, John, thanks for looking at the patch.<br>
Here is the updated webrev:<br>
<br>
<a href="http://cr.openjdk.java.net/~anoll/8027593/webrev.01/" target="_blank">http://cr.openjdk.java.net/~<u></u>anoll/8027593/webrev.01/</a><br>
<br>
Please see comments inline.<br>
<br>
<br>
On 11/06/2013 02:52 AM, John Rose wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Good idea.<br>
<br>
-- John (on my iPhone)<br>
<br>
On Nov 5, 2013, at 3:11 PM, Igor Veresov <<a href="mailto:igor.veresov@oracle.com" target="_blank">igor.veresov@oracle.com</a>><br>
wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Looks good. It’s not related to the change, but could you please<br>
consider adding some printing under PrintMethodFlushing && Verbose<br>
for the case when the method is made not entrant and include hotness<br>
and threshold values?<br>
<br>
igor<br>
</blockquote></blockquote>
I also agree. I added the print.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Nov 5, 2013, at 10:09 AM, Vladimir Kozlov<br>
<<a href="mailto:vladimir.kozlov@oracle.com" target="_blank">vladimir.kozlov@oracle.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 11/5/13 6:44 AM, Albert Noll wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
could I get reviews for this small patch?<br>
<br>
bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8027593" target="_blank">https://bugs.openjdk.java.net/<u></u>browse/JDK-8027593</a><br>
webrev: <a href="http://cr.openjdk.java.net/~anoll/8027593/webrev.00/" target="_blank">http://cr.openjdk.java.net/~<u></u>anoll/8027593/webrev.00/</a><br>
<br>
Problem: The implementation of the sweeper (8020151) causes a<br>
performance regression for small code cache sizes. There<br>
are two issues that cause this regression:<br>
1) NmethodSweepFraction is only adjusted according to the<br>
ReservedCodecacheSize if TieredCompilation is enabled. As a<br>
result, NmethodSweepFraction remains 16 (if TieredCompilation is<br>
not used). This is way too large for small code cache<br>
sizes (e.g., <5m).<br>
2) _request_mark_phase (sweeper.cpp) is initialized to false. As a<br>
result, mark_active_nmethods() did not set<br>
_invocations and _current, which results in not invoking the<br>
sweeper (calling sweep_code_cache()) at all. When<br>
TieredCompilation is enabled this was not an issue, since<br>
NmethodSweeper::notify() (which sets _request_mark_phase) is<br>
called much more frequently.<br>
<br>
Solution: 1) Move setting of NmethodSweepFraction so that it is<br>
always executed.<br>
</blockquote>
Good.<br>
<br>
</blockquote></blockquote></blockquote>
The place where I moved the adaption of NmethodSweepFraction is not<br>
good, since the<br>
the code cache size is adapted later for tiered. The current version<br>
should be fine.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Solution: 2) Remove need_marking_phase(),<br>
request_nmethod_marking(), and reset_nmetod_marking().<br>
I think that these checks are not needed since<br>
8020151, since we do stack scanning of<br>
active nmethods irrespective of the value of<br>
what need_marking_phase() returns. Since<br>
the patch removes need_marking_phase() printing<br>
out the warning (line 327 in<br>
sweeper.cpp) is incorrect, i.e., we continue to<br>
invoke the sweeper. I removed the warning<br>
and the associated code.<br>
</blockquote>
Don't put mark_active_nmethods() under !UseCodeCacheFlushing<br>
condition. We always want to reclaim space in codecache.<br>
</blockquote></blockquote></blockquote>
Done.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
To do nmethod marking at each safepoint is fine (we have to do it<br>
to make sure we did not miss live nmethods). But with your changes<br>
each safepoint triggers sweep. Do we really need sweep so<br>
frequently? Why to sweep if there were no nmethods state change and<br>
there is enough space in CodeCache. So I am not sure about removing<br>
_request_mark_phase, init it with 'true' is okay.<br>
</blockquote></blockquote></blockquote>
I agree. The current patch contains a more sophisticated logic to<br>
determine when we call the<br>
sweeper. The bottom line is that we do not invoke the sweeper only if:<br>
</blockquote>
<br>
!!!!We DO INVOKE the sweeper only if:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(1) The code cache is getting full and/or<br>
(2) There are sufficient state changes in the last sweep.<br>
</blockquote>
!!!!!(3) We have not been sweeping for 'some time'<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The patch contains a detailed description + examples of the logic. I<br>
tested the patch<br>
with small code cache sizes (specjvm98 + <4m code cache), medium-sized<br>
code cache<br>
(128m + nashorn + octane), and large code cache (240m + nashorn +<br>
octane). The measurements<br>
indicate that with the current logic in place, we can reduce the<br>
number of sweeps by 50% for<br>
medium code cache sizes without increasing the maximum used code cache<br>
size. The difference<br>
is more or less not significant.<br>
<br>
Please let me know what you think about it. The main disadvantage I<br>
see with this change is that<br>
it makes reasoning about the sweeper harder than it was before.<br>
<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The warning was useless so it is okay to remove it and<br>
corresponding code.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, I think that we can either remove -XX:MethodFlushing or<br>
-XX:UseCodeCacheFlushing. Since 8020151, one of them is<br>
redundant and can be removed. I am not quite sure if we should do<br>
that now so it is not included in the patch.<br>
</blockquote>
It is for separate change.<br>
<br>
MethodFlushing is obsolete because we can not run VM without<br>
codecache sweeping because we loose performance since we go into<br>
Interpreter after codecache filled. Did you tried to run with it<br>
OFF? I think it is good candidate to go.<br>
<br>
The problem with UseCodeCacheFlushing is it becomes famous so you<br>
can't remove it easily. But don't replace MethodFlushing with it. I<br>
think code which currently guarded by MethodFlushing should be<br>
executed unconditionally.<br>
<br>
In arguments.cpp there is table for obsolete flags:<br>
static ObsoleteFlag obsolete_jvm_flags[] = {<br>
<br>
jdk8 is major release so we can change flags. Add MethodFlushing<br>
there to remove it in jdk9:<br>
{ "MethodFlushing", JDK_Version::jdk(8), JDK_Version::jdk(9) },<br>
<br>
Thanks,<br>
Vladimir<br>
</blockquote></blockquote></blockquote>
I'll file a new bug to remove the flag. I guess this change will most<br>
likely only make it into 8uXX.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Testing<br>
bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8027593" target="_blank">https://bugs.openjdk.java.net/<u></u>browse/JDK-8027593</a> also shows a<br>
performance evaluation.<br>
<br>
Many thanks for looking at the patch.<br>
Best,<br>
Albert<br>
</blockquote></blockquote></blockquote></blockquote>
<br>
</blockquote>
<br>
</blockquote></blockquote></blockquote>
<br>
</blockquote></div>