<div dir="ltr"><div class="gmail_quote"><div dir="ltr">+hotspot-gc-dev<br></div><div dir="ltr"><br></div><div dir="ltr">On Thu, Mar 15, 2018 at 1:25 AM Thomas Schatzl <<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
On Thu, 2018-03-15 at 01:00 +0000, Ian Rogers wrote:<br>
> An old data point on how large a critical region should be comes from<br>
> java.nio.Bits. In JDK 9 the code migrated into unsafe, but in JDK 8<br>
> the copies within a critical region were bound at most copying 1MB:<br>
> <a href="http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/" rel="noreferrer" target="_blank">http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/</a><br>
> native/java/nio/Bits.c#l88 This is inconsistent with Deflater and<br>
> ObjectOutputStream which both allow unlimited arrays and thereby<br>
> critical region sizes.<br>
><br>
> In JDK 9 the copies starve the garbage collector in nio Bits too as<br>
> there is no 1MB limit to the copy sizes:<br>
> <a href="http://hg.openjdk.java.net/jdk/jdk/rev/f70e100d3195" rel="noreferrer" target="_blank">http://hg.openjdk.java.net/jdk/jdk/rev/f70e100d3195</a><br>
> which came from:<br>
> <a href="https://bugs.openjdk.java.net/browse/JDK-8149596" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8149596</a><br>
><br>
> Perhaps this is a regression not demonstrated due to the testing<br>
> challenge.<br>
> [...]<br>
> It doesn't seem unreasonable to have the loops for the copies occur<br>
> in 1MB chunks but JDK-8149596 lost this and so I'm confused on what<br>
> the HotSpot stand point is.<br>
<br>
Please file a bug (seems to be a core-libs/java.nio regression?),<br>
preferably with some kind of regression test. Also file enhancements (I<br>
would guess) for the other cases allowing unlimited arrays.<br></blockquote><div><br></div><div>I don't have perms to file bugs there's some catch-22 scenario in getting the permissions. Happy to have a bug filed or to file were that not an issue. Happy to create a test case but can't see any others for TTSP issues. This feels like a potential use case for jmh, perhaps run the benchmark well having a separate thread run GC bench.</div><div><br></div><div>Should there be a bug to add, in debug mode, a TTSP watcher thread whose job it is to bring "random" threads into safepoints and report on tardy ones?</div><div>Should there be a bug to warn on being in a JNI critical for more than just a short period?</div><div>Seems like there should be a bug on Unsafe.copyMemory and Unsafe.copySwapMemory having TTSP issues.</div><div>Seems like there should be a bug on all uses of critical that don't chunk their critical region work based on some bound (like 1MB chunks for nio Bits)? How are these bounds set? A past reference that I've lost is in having the bound be the equivalent of 65535 bytecodes due to the expectation of GC work at least once in a method or on a loop backedge - I thought this was in a spec somewhere but now I can't find it. The bytecode size feels as arbitrary as 1MB, a time period would be better but that can depend on the kind of GC you want as delays with concurrent GC mean more than non-concurrent. Clearly the chunk size shouldn't just be 0, but this appears to currently be the norm in the JDK.</div><div><br></div><div>The original reason for coming here was a 140x slow down in -Xcheck:jni in Deflater.deflate There are a few options there that its useful to enumerate:</div><div>1) rewrite in Java but there are correctness and open source related issues</div><div>2) remove underflow/overflow protection from critical arrays (revert <span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">JDK-6311046 or perhaps bound protection to arrays of a particular small size) - this removes checking and doesn't deal with TTSP</span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">3) add a critical array slice API to JNI so that copies with -Xcheck:jni aren't unbounded (martinrb@ proposed this) - keeps checking but doesn't deal with TTSP</span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">4) rewrite primitive array criticals with GetArrayRegion as O(n) beats the "silent killer" TTSP (effectively deprecate the critical APIs)</span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div>In general (ie not just the deflate case) I think (1) is the most preferable. (2) and (3) both have TTSP issues. (4) isn't great performance wise, which motivates more use of approach (1), but I think deprecating criticals may just be the easiest and sanest way forward. I think that discussion is worth having on an e-mail thread rather than a bug.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Long TTSP is a performance bug as any other.<br>
<br>
> In a way criticals are better than unsafe as they may<br>
> pin the memory and not starve GC, which shenandoah does.<br>
<br>
(Region based) Object pinning has its own share of problems:<br>
<br>
 - only (relatively) easily implemented in region based collectors<br>
<br>
 - may slow down pause a bit in presence of pinned regions/objects (for<br>
non-concurrent copying collectors)<br>
<br>
 - excessive use of pinning may cause OOME and VM exit probably earlier<br>
than the gc locker. GC locker seems to provide a more gradual<br>
degradation. E.g. pinning regions typically makes these regions<br>
unavailable for allocation.<br>
I.e. you still should not use it for many, very long living objects.<br>
Of course this somewhat depends on the sophistication of the<br>
implementation.<br>
<br>
I think region based pinning would be a good addition to other<br>
collectors than Shenandoah too. It has been on our minds for a long<br>
time, but there are so many other more important issues :), so of<br>
course we are eager to see contributions in this area. ;)<br>
<br>
If you are interested on working on this, please ping us on hotspot-gc-<br>
dev for implementation ideas to get you jump-started.<br>
<br>
Thanks,<br>
  Thomas<br></blockquote><div><br></div><div>I'd rather deprecate criticals than build upon the complexity, but I'm very glad this is a concern.</div><div><br></div><div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">Thanks,</div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">Ian</div> </div></div></div>