[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR 8159638: Improve array caches and renderer stats in Marlin renderer
From:       Jim Graham <james.graham () oracle ! com>
Date:       2016-07-21 21:41:34
Message-ID: 5480c2b9-a8fd-bc42-6e75-1b17c129dd59 () oracle ! com
[Download RAW message or body]

Hi Laurent,

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

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

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.

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

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

			...jim
[prev in list] [next in list] [prev in thread] [next in thread]