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

[Laurent Bourgès]

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

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