<div dir="ltr"><div>Dear Jim,<br><br></div>Thanks for your review, I'll answer your questions in the text below and will later propose a new webrev:<br><br><div><div><div class="gmail_extra"><br><div class="gmail_quote">2016-07-19 9:00 GMT+02:00 Jim Graham <span dir="ltr"><<a href="mailto:james.graham@oracle.com" target="_blank">james.graham@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Some work should be done on the comments at the top of ArrayCache.java - line 38 and 42 make the same claim about 2 different thresholds.</blockquote><div><br></div><div>I agree THRESHOLD_LARGE_ARRAY_SIZE can be removed as THRESHOLD_ARRAY_SIZE = THRESHOLD_LARGE_ARRAY_SIZE now !<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

It seems silly, but in ArrayCache.getNewLargeSize(), lines 162 and 164 are both ">" tests and then the newly added test at 166 is a "<" test.  It would be nice to main symmetry of the tests in that if/then/elseif sequence.<br></blockquote><div><br></div><div>Will do.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

As far as I can see, the buckets are:<br>
<br>
0 - 4K<br>
1 - 16k<br>
2 - 64k<br>
3 - 256k<br>
4 - 1m<br>
5 - 4m<br>
6 - 8m<br>
7 - 16m<br>
<br>
which makes MAX_ARRAY_SIZE == THRESHOLD_ARRAY_SIZE == 16m and THRESHOLD_LARGE is also 16m...?<br></blockquote><div><br></div><div>As theses values are all equals now, I will only keep MAX_ARRAY_SIZE (= maximum size of cached array in buckets) and fix the getNewSize() and getNewLargeSize() to use that value as the threshold.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I don't have any issues with those numbers, but the way that they are calculated makes it look like they are supposed to be unique numbers and yet through the obscurity of the loops used to populate the sizes they just end up all being the same numbers - it makes me wonder if there was a mistake in there or not...?<br></blockquote><div><br></div><div>Initially these values have different values / meanings but it can be simplified now.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

CacheStats.reset() - should totalInitial be reset as well?  Also, there should be a reset method on the BucketStats class rather than just digging in and modifying its members directly from outside the class.<br></blockquote><div><br></div><div>Initial arrays are allocated by the Reference constructor so totalInitial must not be reset to give the total memory allocated by initial arrays.<br></div><div>Will add the BucketStats.reset() method.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

It feels odd to have all of the cache classes import the static members of ArrayCache rather than just subclassing it. Is there a reason it wasn't just subclassed?<br></blockquote><div><br>As all the members are static, I prefer having an explicit static import instead of subclasses. <br>Moreover, I deliberately avoided any class inheritance to avoid the performance penalty of bimorphic / megamorphic method calls (abstract or overriden methods).<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

The Dirty and Clean subclasses are nearly identical and yet they share no code simply because buried inside one of their inner classes one of them clears the data and the other does not.  That seems a waste for something so trivial in the design.<br></blockquote><div><br></div><div>The only reason is performance: I want to ensure straightforward method calls ie no bimorphic or virtual calls to be inlined by hotspot. Maybe such fear or approach is too conservative i.e. the performance penalty is too small to consider such optimization. I made many tests with jmh (in june) but I agree the design can be simpler:<br></div><div>- abstract [Byte/Int/Float]ArrayCache (where putArray() is abstract)<br></div><div>- Clean[Byte/Int/Float]ArrayCache and Dirty[Byte/Int/Float]ArrayCache implements properly the putArray method but also the createArray() method (new array or Unsafe.allocateUninitializedArray)<br></div><div><br></div><div>I could try again but I prefer the current design as it is (overly) strongly typed.<br></div><div>Please propose an alternative / simpler design !<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

The various Reference.putArray() methods protect against putting the initial arrays, and you the code that calls them also makes the same check.  I'd remove the code that checks for initial from the callers so that the call sites are more streamlined, but there's no functional issue with the way it is now.<br></blockquote><div><br></div><div>I will improve that and that will save several tests / lines. <br>FYI I adopted the double-checks to promote the initial array case as the fast path whereas the array cache is the slow path (less probable / exceptional). Typically hotspot optimizes very well such code when only initial arrays are in use (general use case).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

Also, since you never put the initial arrays, they aren't automatically cleaned...?<br></blockquote><div><br></div><div>Exactly: initial arrays are allocated by the Reference class and directly used by callers (fill / clean) and the XxxArrayCache never touch them.<br><br>Only CleanIntArrayCache.Reference are used by Marlin:<br>- MarlinCache<br>  116:  touchedTile_ref = rdrCtx.newCleanIntArrayRef(INITIAL_ARRAY); // 1K = 1 tile line<br>- Renderer<br>  549:  edgeBuckets_ref      = rdrCtx.newCleanIntArrayRef(INITIAL_BUCKET_ARRAY); // 64K<br>  550:  edgeBucketCounts_ref = rdrCtx.newCleanIntArrayRef(INITIAL_BUCKET_ARRAY); // 64K<br>  556:  alphaLine_ref = rdrCtx.newCleanIntArrayRef(INITIAL_AA_ARRAY); // 8K<br>  571:  blkFlags_ref = rdrCtx.newCleanIntArrayRef(INITIAL_ARRAY); // 1K = 1 tile line<br><br></div><div> All cases are manually cleaned in MarlinCache.resetTileLine(), Renderer.dispose() and MarlinCache.copyAARowNoRLE...() for alphaLine and blkFlags arrays with several cleanup patterns (optimized and performance-critical).<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

Is the shell script only used by you to replicate changes from the Byte classes to the Int/Float classes?  A shell comment at the top would be nice to explain that...<br></blockquote><div><br></div><div>Yes. Will do. <br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

The only issue above is whether or not the initial arrays are missing a clean pass on them due to the fact that they get rejected by the put methods.  All of the others are simply comments to engage some discussion on the design elements...<span><font color="#888888"><br></font></span></blockquote><div><br></div><div>Please give me your feedback and discuss the design (with performance considerations in mind).<br><br></div><div>Cheers,<br></div><div>Laurent<br></div><br></div></div></div></div></div>