[OpenJDK Rasterizer] Marlin #4

[Jim Graham]

Hi Laurent,

On 11/23/15 9:02 AM, Laurent Bourgès wrote:
> I know that Marlin is slightly slower than ductus for shape size ~ 20:
> Ductus seems using 16x16 blocks whereas Marlin uses 32x32 tiles so the
> new RLE approach is not in use (raw encoding) i.e. lots of zero-fill /
> array copy operations.

What makes you think that Ductus is using 16x16 blocks?  It is designed 
around 32x32 tiles to the point where it isn't really configurable (they 
were modeling what they thought was going to be AA rendering hardware 
that would take over the world in the mid-90s and it never happened). 
We did try different block sizes when we were integrating the 
technology, back when they were still designing the hardware chips, but 
once we settled on 32x32 they baked it in like it was hardware.

> Finally Marlin has still a disavantage: it does not perform shape
> clipping in contrary to ductus = it's a remaining TODO.
>
>     I'm code reading now:
>
>     ArrayCache.java, line 205 - should that be needSize there?  Also,
>     should these tests be > or >=?
>
> I wanted to limit the size to 2M (Integer.MAX_VALUE) but it wanted 2
> passes: first, return 2M, then if more needed, fail !
> If prefer using >= to be simpler ie always check the current size.

The point is that the hard failure is a condition of when we need more 
than we can provide, not when we "already have" more than we can 
provide.  needSize should cause the hard failure, not the current size. 
  And if needSize is going to cause the hard failure then why use >= 
instead of > - we can supply MAX_INT if that is what is "needed".

Also, there is a potential bug right now that curSize could be < 
MAX_INT, but needSize could be > MAX_INT, but the test at line 205 and 
the adjustment at line 212 will decide that we can return with MAX_INT 
being the answer - even though that is less than what we really need.

It is just much more straightforward to have the tests be:

- if we are planning to allocate more than MAX
    (that isn't going to work, so...)
    - then if we really *need* more than MAX => exception
    - otherwise just allocate MAX

Another way to code this would be to first clip against MAX as being a 
hard limit and then throw an exception if the planned amount "isn't enough":

if (size > MAX_INT) size = MAX_INT;
if (size < needSize) exception("We couldn't allocate enough");
return size;

>     Renderer.java, line 1446 - initial value of useBlkFlags will be
>     false if heuristics is enabled
>     Renderer.java, line 1329 - we send a row to the cache before we
>     update heuristics at line 1332
>     - taken together that means that with heuristics the very first
>     cache line sent out will always be non-RLE?
>
>
> You're right:
> In my first tests, I was testing every scanline but it was costly (8x
> more tests but always clean blkFlags when used).
> Now I only check 1/8th scanlines ie once per pixel row as it is roughly
> "continuous" (maybe false positive) and it implies the very first row is
> always non RLE encoded.

Is this intentional?  Or can the heuristic be done at the beginning so 
that the first pixel row also benefits from it?

>     Once enableLogs is on, isUseLogging controls whether it goes to a
>     log file or just prints to System.out.
>
>     Also, there are other static booleans turn on and off different
>     pieces of the logging at a finer granularity.  The only logging that
>     was happening without being wrapped by a "if (doFooLogging)"
>     conditional were the logInfos that executed when the RE was
>     initialized.  I suppose those could have been neutered with a
>     different "doStartupInfo" boolean, but I figured I would turn them
>     all off by default for now.
>
>     We can enable runtime-logging later on if we find a decent way to
>     have it be low impact when not runtime-enabled...
>
>
> (I deliberately disabled doMonitors and doChecks (for performance reasons))
>
>
> However I would prefer having enableLogs = true if the renderer is in
> verbose mode: "  -Dsun.java2d.renderer.verbose=true".
>
> So it would display the startup informations in this case (including
> tuning settings ...) and it would allow gathering Marlin statistics (if
> doStats = true).

I could see that, but perhaps it should be controlled by a 
marlin-specific attribute?

In any case, this is very simple to add later once we get this 
integrated.  I just wanted to be sure that initial integration took the 
path of least resistance to silencing it.

> Moreover, I should refactor j.u.l.Logger usage by PlatformLogger (TODO)
> and we should rename all Marlin System properties to use the prefix
> "sun.java2d.marlin..."

I would support that for marlin-specific properties...

		...jim