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.gra...@oracle.com
<mailto:james.gra...@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
