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
