Dear Jim, Thanks for your review, I'll answer your questions in the text below and will later propose a new webrev:
2016-07-19 9:00 GMT+02:00 Jim Graham <james.gra...@oracle.com>: > > 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. I agree THRESHOLD_LARGE_ARRAY_SIZE can be removed as THRESHOLD_ARRAY_SIZE = THRESHOLD_LARGE_ARRAY_SIZE now ! > 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. > Will do. > As far as I can see, the buckets are: > > 0 - 4K > 1 - 16k > 2 - 64k > 3 - 256k > 4 - 1m > 5 - 4m > 6 - 8m > 7 - 16m > > which makes MAX_ARRAY_SIZE == THRESHOLD_ARRAY_SIZE == 16m and > THRESHOLD_LARGE is also 16m...? > 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. > 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...? > Initially these values have different values / meanings but it can be simplified now. > 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. > Initial arrays are allocated by the Reference constructor so totalInitial must not be reset to give the total memory allocated by initial arrays. Will add the BucketStats.reset() method. > 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? > As all the members are static, I prefer having an explicit static import instead of subclasses. Moreover, I deliberately avoided any class inheritance to avoid the performance penalty of bimorphic / megamorphic method calls (abstract or overriden methods). > 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. > 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: - abstract [Byte/Int/Float]ArrayCache (where putArray() is abstract) - 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) I could try again but I prefer the current design as it is (overly) strongly typed. Please propose an alternative / simpler design ! > 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. > I will improve that and that will save several tests / lines. 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). > Also, since you never put the initial arrays, they aren't automatically > cleaned...? > Exactly: initial arrays are allocated by the Reference class and directly used by callers (fill / clean) and the XxxArrayCache never touch them. Only CleanIntArrayCache.Reference are used by Marlin: - MarlinCache 116: touchedTile_ref = rdrCtx.newCleanIntArrayRef(INITIAL_ARRAY); // 1K = 1 tile line - Renderer 549: edgeBuckets_ref = rdrCtx.newCleanIntArrayRef(INITIAL_BUCKET_ARRAY); // 64K 550: edgeBucketCounts_ref = rdrCtx.newCleanIntArrayRef(INITIAL_BUCKET_ARRAY); // 64K 556: alphaLine_ref = rdrCtx.newCleanIntArrayRef(INITIAL_AA_ARRAY); // 8K 571: blkFlags_ref = rdrCtx.newCleanIntArrayRef(INITIAL_ARRAY); // 1K = 1 tile line 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). > 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... > Yes. Will do. > 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... > Please give me your feedback and discuss the design (with performance considerations in mind). Cheers, Laurent
