[OpenJDK Rasterizer] RFR: Marlin renderer #2

Fri Jun 12 12:45:07 UTC 2015

Hi Jim,

Here is my first comments:

2015-06-12 1:43 GMT+02:00 Jim Graham <james.graham at oracle.com>:

> Hi Laurent,
>
> I reviewed the buffer management code this time.
>

Thanks.

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

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

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 !

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

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

It is defensive programming to avoid sharing initial arrays in caches at
two steps:
1/ array length must be even and < MAX:
  void putDirtyIntArray(final int[] array) {
        final int length = array.length;
*        if (((length & 0x1) == 0) && (length <= MAX_ARRAY_SIZE)) {*
            getDirtyIntArrayCache(length).putDirtyArray(array, length);
        }
    }

2/ length must match the cache's array length:
    void putDirtyArray(final int[] array, final int length) {

*        if (doChecks && (length != arraySize)) {
System.out.println("bad length = " + length);            return;        }*
...

I tested intensively all that code with *doChecks=true* (development) and
disabled it for production.
If you prefer, I can always enable this check if you prefer.

I wanted to have a simple but robust API that can be easily used in Marlin.

To conclude, the cache only accepts arrays having the proper size to NEVER
share an initial array and detect any bug in cache usage.

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];
}

Anyway I agree the cache handling can be improved a lot: try using generics
for array types with a lambda factory to allocate them.

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

I understand your feeling and I am still here to clean it, make it more
robust ...

I can provide some test code: the SpiralTest class creates a single insane
shape to exceed all initial arrays and validate all caches get/grow/put
methods.

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

Let's stay concrete: please point me the obscure places in the code that I
could improve / refactor.

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

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

To allocate arrays, I need to implement an ArrayAllocator: K create(int
length) for every concrete array types.

What do you mean by "meking further real improvements in the algorithms" ?

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

OK, will do.

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

OK

> 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
>
The test (slope == pslope) is used to compare Infinity values that are not
handled in the other test.
I will add a comment.

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

I agree it may be a problem: to be improved later.

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

I understand your point of view and it is for now only in the
graphics-rasterizer repository and we have  time to improve it before it
goes into jdk9 repository.

Let's focus on fixing / documenting the most obscure (tricky) aspect until
we have a new array cache framework.

Who could help ? I am the only one working concretely on this code, help
wanted.

Thanks for your review,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/graphics-rasterizer-dev/attachments/20150612/ca3ec2db/attachment-0001.html>