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