[OpenJDK Rasterizer] RFR: Marlin renderer #2

[Jim Graham]

OK, the new array stuff looks good to go.  There is still those 2 minor 
issues from before, but that's it:

CollinearSimplifier.getNativeConsumer() => return 0
FloatMath => parens around (~intpart) at 145.

I don't need to see a webrev for those changes...

			...jim

On 6/17/15 1:43 PM, Jim Graham wrote:
> Hi Laurent,
>
> There were 1 or 2 very minor fixes I suggested in a couple of files that
> you indicated you were going to do, but I didn't see a new webrev.  Is
> there one?  I think the existing array stuff is probably OK and I'll
> look at it again now that I understand the tradeoffs of the current
> design of it, but I wanted to know if there was a newer webrev for me to
> review based on those other minor fixes?
>
> On 6/12/15 5:45 AM, Laurent Bourgès wrote:
>>     A general thought - should we have foo_initial arrays if we are
>>     getting everything from a cache?
>>
>> I adopted this approach to maximize performance for the 99% cases: I
>> sized properly the initial arrays and they takes ~ 512Kb per
>> RendererContext: the ArrayCache (Weak) is only used for extreme cases (a
>> bit slower). In march 2013, I started by using the cache for all array
>> accesses and it was 30% slower !
>
> Was the 30% back when we were allocating or clearing all of the arrays?
>   Now that we only clear what we use, the initial sizes of the arrays
> shouldn't matter so much.  It would be worth a little extra size
> overhead to not have so many special cases and special handling for the
> "_initial" arrays if the performance is now essentially identical.
>
>> I will look again at these cases: I agree "leaking refs" should be set
>> to null to be sure. However, Marlin code is always executing the pattern
>> init() / dispose() so leaking pointers are not critical but OK let's set
>> all of them to null.
>>
>> For initial arrays, it seems redundant to set to null in dispose() and
>> set to initial in init() = useless and counter-productive !
>
> It gets easier to review and maintain code if there is a clear reset to
> some initial conditions is all.  I would like to try to keep the code as
> simple and straightforward as possible while evaluating algorithmic
> improvements without introducing potential bugs with a lot of special
> case code and then we can worry about the 1%'s later.
>
> Also, we may find moving the code to native to be more of a performance
> improvement than trying to fight with Hotspot's load/store policies and
> so any code that tries to make "slightly more optimal byte codes" would
> end up not being needed and would add some noise to any such native
> conversion.
>
> But, I do agree that technically you are correct about some of these
> suggestions leading to some provably redundant code.  The issue is more
> that constantly having to reprove the redundancies gets in the way of
> some other progress we could be making.
>
> And, on the other hand, right now you are the engineer driving this from
> the workload point of view (and I apologize if I am inadvertently in a
> role that seems to be described as "the engineer getting in the way"),
> so I'm willing to rely on your commitment to be diligent about these
> optimizations.  I'm just stating a case for whether they should be the
> focus yet.
>
>> Sorry it is not a flaw but an advantage: the initial arrays that can
>> grow in widenXXXArray() have an odd size as indicated by the comment:
>>      // +1 to avoid recycling in Helpers.widenArray()
>>      private final int[] edgePtrs_initial  = new int[INITIAL_SMALL_ARRAY
>> + 1]; // 4K
>
> I saw that after the fact, but my main point remains that this is a
> fragile inter-module optimization.  I was unaware that it could save us
> 30% to have special-cased "initial" arrays, and that fact makes this
> relationship that much more important but I wonder if that is still
> true.  If we ditch the initial arrays because the new "dirty array"
> management removes that performance penalty then this all goes away.
> And, even if we keep the initial array concept it would be nice to find
> a more reliable way to manage that than a side effect of its length.
>
>> I wanted to have a simple but robust API that can be easily used in
>> Marlin.
>
> I'd like to think that I'm working/promoting towards simple and robust
> APIs as well.
>
>> I agree the documentation can be improved a lot and I could introduce a
>> new helper method in ArrayCache to create the initial arrays in a safer
>> and explicit way :
>>
>> int[] createNonCachedIntArray(int length) {
>> // return only odd length to detect wrong cache usage
>>      if (length & 1 == 0) {
>>          length +=1;
>>      }
>>      return new int[length];
>> }
>
> In a future effort, if we find that we must keep some small initial
> arrays for performance, we could make this a more explicit and tunable
> parameter on the caches.  Let's table that until we consider if they buy
> us any performance after these dirty array changes.
>
>> Anyway I agree the cache handling can be improved a lot: try using
>> generics for array types with a lambda factory to allocate them.
>
> I just wish that generics could handle primitive arrays, but they can't.
>   I did a similar generification thing for the raster format conversion
> code in FX, but I had to play games with the generic types being
> ByteBuffer and IntBuffer and having the variants of those accept the
> proper type of primitive arrays.  It wasn't pretty, but it did work out
> in the end.
>
>>     The caching code is an example of "code with no obvious flaws
>>     because it is hard to figure out if it has flaws".  We should strive
>>     for "code with obviously no flaws because it is easy to verify that
>>     it is doing everything correctly".
>>
>>
>> I disagree the term "flaw" but I agree it is hard to figure out what is
>> done.
>
> I took that from a talk on producing more secure code.  I think it
> originally came from an overall system design that could easily be
> proven secure.  The first release of Java required all sensitive Java
> code to be constantly asking "am I in the context that this operation is
> allowed?" and the detection was fragile.  It was replaced by code where
> all sensitive operations were rejected unless the surrounding code
> proved its protected context which was much more secure.  "Disallowed
> unless requested" was harder to make a mistake than "allowed unless it
> looks questionable".
>
> But, that same philosophy serves a lot of code well.  If you start
> having a bunch of "agreements" between modules then they can quickly get
> out of synch through regular maintenance and sometimes spiral out of
> control and you spend more time verifying that you're still satisfying
> all of these conventions rather than letting the code make sure things
> are kosher because only valid operations are expressable.
>
>> OK, I can work later on a new framework:
>> - ArrayCache<K> abstract class where <K> is byte[], int[], float[]
>> - CleanArrayCache<K> extends ArrayCache<K> = clean arrays
>> - DirtyArrayCache<K> extends ArrayCache<K> = dirty arrays
>
> I think I can trust the existing architecture for at least a putback of
> the last webrev that you sent out while we braingstorm a better caching
> mechanism - let me make one last pass through it and I'll send an OK
> later this afternoon.
>
>> What do you mean by "meking further real improvements in the
>> algorithms" ?
>
> Things like investigating DDA in the inner loops, alternate ways to
> manage crossings that require less sorting, enhancing the stroker to do
> non-uniform scaling (which would eliminate the double transform in some
> cases), potentially passing the crossings to the output loop rather than
> having to convert them into an alpha array, etc.  A lot of that is
> easier to review if there aren't a lot of extra "this line added 1%
> performance" code adding visual noise.
>
>> Who could help ? I am the only one working concretely on this code, help
>> wanted.
>
> I'd love to get more people involved, but history tells us that this is
> a fairly lonely field.  :(
>
> I may have some time now that FX is in tightened bug-fix mode to do some
> tinkering, but mainly I will probably be much quicker on getting reviews
> done which may help as much as having another pair of hands in practice...
>
>              ...jim