[OpenJDK Rasterizer] RFR: Marlin renderer #2

[Jim Graham]

Hi Laurent,

I reviewed the buffer management code this time.

A general thought - should we have foo_initial arrays if we are getting 
everything from a cache?

Another general thought - in places you have a reset/dispose method that 
puts the array back in the cache and then resets the array pointer to 
the _initial version, but in other places that do not have an _initial 
version you put the array back and then leave the pointer alone. 
Shouldn't the "non-_initial" places set the associated array to null to 
make sure we don't leak a previously cached array into a new operation? 
  For that matter, perhaps they should all be returned to null anyway 
and rely on the constructor to initialize them to the "_initial" variant 
where appropriate?

Renderer.java line 824 - I think you leak "edgePtrs_initial" here to the 
cache...?
(Later I noticed some code in the RenderContext that does special 
handling of arrays with odd sizes, but if that is a workaround for the 
fact that you are leaking an "initial" array to the cache, I find that 
alarmingly fragile and a more robust and obvious solution needs to be 
found here...)

All in all, I find the caching code here to be alarmingly fragile and 
non-obvious.  A better mechanism needs to be found here that does not 
require so much agreement of assumptions between various classes.  It is 
really hard to faithfully believe that you've squashed all the bugs 
here.  We can test, but bugs that are caused by interdependent 
mechanisms like this cache code can be so hard to reproduce that we may 
not discover issues until long after deployment.  And, even if we test 
now - every single change made to the code here will invalidate all of 
our confidence that the interdependencies have remained valid through 
the testing.

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

The primary advantage it provides - that of reuse of arrays and a 
sensible growth strategy - can be provided with a simpler interface that 
is easier to vet and review.  I would argue that even if we came up with 
such an interface and it cost us 1% or 2% in performance now we would 
buy that back quickly over time in the amount of time we would save in 
making further real improvements in the algorithms without having to 
constantly worry about how we might disrupt a fragile caching scheme.

On 6/10/15 2:00 PM, Laurent Bourgès wrote:
> Jim,
>
> Here is the new webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-s2.2/

I just noticed in CollinearSimplifier.getNativeConsumer() - that should 
return 0 because otherwise some code that uses this could decide that it 
is going to call your native consumer directly and bypass all of your 
code.  Basically, unless there is a native version of the code in 
CollinearSimplifier, and it implements the native interfaces required 
for direct native-to-native calling, you should return 0.  On the other 
hand, I don't think this hurts anything because it should only be called 
before any work is done and if you allow the native caller to forward to 
your delegate's native consumer then all we'd miss out on would be the 
simplification done here.  Also, I don't think this gets used with any 
down-stream consumers that have a native consumer so this would 
essentially be "return someone else's 0 instead of my own hard-coded 0", 
but for purity, this should really be "return 0;"...

FloatMath, style note - personally I'd add parentheses around "~intpart" 
at line 145 since I don't like to write code that assumes that the 
reader knows the order of operations for Java code for anything beyond 
"+-*/" and parentheses.

A couple thing to note for later:

CollinearSimplifier.lineTo(PreviousLine case):
- abs(slope - pslope) and "slope == pslope" both involve a virtual 
subtraction of the two values, hopefully the compiler can optimize that out
- Slope of new segment compared to slope of original line segment isn't 
sufficent, but it will likely work reasonably well in practice. 
Consider two initial points with a slope of 0.0 (horizontal).  Now add a 
million short segments with a slope of -EPS.  You can reach an arbitrary 
negative Y value with those.  Now add a million short segments with a 
slope of +EPS.  That can eventually bring you back to y==0.  Now trigger 
a drawLine and you will draw a horizontal line.  But, the accumulated 
segments could have reached any arbitrary negative Y displacement that 
will be completely ignored in the end.

> What do you like me to work on in the step #3 ?
> - DDA ?
> - new Array Cache hierarchy ?
> - Stroker improvements to reduce collinear segments (caps, joins) ?

Unfortunately, I think we need a new caching mechanism sooner rather 
than later.  The current code is not maintainable.  It may be somewhat 
maintainable by you right now, but it will not remain so for long and it 
will not be maintainable by the next engineer that needs to work on this 
code.

				...jim