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

[Jim Graham]

Hi Laurent,

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.

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.

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...?

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...?

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.

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?

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 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.

Also, since you never put the initial arrays, they aren't automatically cleaned...?

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...

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...

			...jim

On 6/15/16 2:10 PM, Laurent Bourgès wrote:
> Hi,
>
> Please review this bug fix for the Marlin renderer to improve the array caches, its usages but also the renderer stats:
> bug: https://bugs.openjdk.java.net/browse/JDK-8159638
> webrev: http://cr.openjdk.java.net/~lbourges/marlin/marlin-8159638.0/
>
> This patch also reduces slightly the memory footprint: 1 RendererContext represents now ~ 450K (310K on-heap / 140K
> off-heap).
>
> Changes:
> - Byte/Int/Float ArrayCache removed
> - replaced by Clean[Byte/Int/Float]ArrayCache (zero-filled arrays) and Dirty[Byte/Int/Float]ArrayCache classes (dirty
> arrays). These new classes provides a Reference class that wraps the initial array and acts as a proxy to the related
> array cache instance (get/widen//put Array)
>
> - ArrayCache: increased bucket to 8 (larger retained memory but weakly referenced) + added CacheStats (and BucketStats)
>
> - MarlinProperties: added setiings for initial edge capacity and align array sizes to 64 (power of 2)
>
> - RendererContext: large cleanup + use a weak reference for the recycled Path2D instance
>
> - RendererStats: big refactoring to create one RendererStats instance per created RendererContext instance (thread
> stats) and the new RendererStatsHolder class gathers all RendererStats instances to dump them at shutdown (very small
> retained memory instead of keeping all RendererContext instances)
>
> Tested with current jtreg tests (+ my MapBench tests)
>
> Regards,
> Laurent