[OpenJDK Rasterizer] Marlin #4

[Laurent Bourgès]

Hi Jim,

>>     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 seems you are right: there is a potential remaining failure !
I tested my code in CrashTest but it passed as the off heap growth
exponentially ie the mentioned case never happened !

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/graphics-rasterizer-dev/attachments/20151123/cd750c34/attachment-0001.html>