[OpenJDK 2D-Dev] RFR 8159638: Improve array caches and renderer stats in Marlin renderer

Thu Jul 21 13:56:55 UTC 2016

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.graham at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20160721/f7395e26/attachment.html>