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

[Jim Graham]

Hi Laurent,

A tip on webrevs, if you supply a file of filenames then you can tell it 
to diff a file with a name change against its former name by using 2 
filenames on the same line, as in:

--------
filetodiff1.java
filetodiff2.java
filetodiff3.java filetodiff3oldname.java
filetodiff4.java
--------

In Renderer.java, you create the alphaLine and blkFlags refs as Clean, 
but then you always put them back using indices of (0, 0) so they will 
never actually be cleaned - is there a reason you don't just use a dirty 
ref there?

Other than that question, I don't see any problems with the fix...

			...jim

On 08/01/2016 02:17 PM, Laurent Bourgès wrote:
> Hi Jim,
>
> Here is an updated webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8159638.1/
>
> Changes:
> - merge Clean/Dirty Xxx ArrayCache using the flag 'clean' to indicate if
> the cache is clean or dirty + the putArray method always performs the
> array cleanup
> - ArrayCache renamed to ArrayCacheConst + simplified thresholds
>
> More comments, below:
>
> 2016-07-21 23:41 GMT+02:00 Jim Graham <james.graham at oracle.com
> <mailto:james.graham at oracle.com>>:
>
>
>     On 07/21/2016 06:56 AM, Laurent Bourgès wrote:
>
>             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.
>
>
>     To be clear, I wasn't complaining about the multitude of thresholds,
>     but rather that the way they were apportioned - sort of as a side
>     effect of the computations in a loop - ended up accidentally
>     squashing a couple of them out of existence.  Another option would
>     be to make sure that the thresholds make sense, but keep all 4
>     threshold ranges, but you are the one who knows the details of how
>     these sizes impact performance...
>
>
> Simplified a bit + fixed comments.
>
>
>
>             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).
>
>
>     First, I would have expected MumbleArrayCache classes to be a
>     subclass of the ArrayCache class in the first place.  If it is not
>     going to be the base class then it should have the name
>     "ArrayCacheUtils" or "Const" or something.
>
>
> Renamed to ArrayCacheConst.
>
>
>
>     Second, wildcard imports are recommended against.
>
>     Finally, if you are going to use static methods from another class I
>     would much rather see explicit naming rather than importing them.
>     While static imports work for methods as well as constants, they are
>     typically used for constants and it is confusing to apply them to
>     methods.
>
>
> Fixed.
>
>
>         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 !
>
>
>     That's something that can be investigated later as it would be a big
>     disruption for the current task.  Generally, though, as long as the
>     leaf classes are final and the cache classes are strongly typed then
>     HS should inline freely.
>
>
> I adopted a simple boolean flag 'clean' to perform the optional cleanup
> and renamed classes to Byte/Float/Int ArrayCache.
>
>
>
>             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.
>
>
>     What I was getting at was that this was potentially a bug?  If you
>     carry over use of an initial array in a clean setting without
>     calling to the cache object, then who clears the used portion?
>
>          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).
>
>
>     I see.  Is this really a noticeable performance issue to rely on the
>     cache to do the cleaning and have much more readable code?
>
>
> I adopted your proposal and simplified the code in Renderer: the
> Reference.putArray() clears the array portion if needed in all cases.
> The performance impact is very low: only 3% when the shape count is huge.
>
>
> Cheers,
> Laurent