[OpenJDK Rasterizer] Fwd: Re: Fwd: RFR: Marlin renderer #3

Mon Aug 17 21:34:24 UTC 2015

Jim & Phil

Here is a new webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-s3.4/

Changes:
- Renderer: remove unused D_ERR_STEP_MAX and 0x1 replaced by 1 for
readability
- MarlinRenderingEngine: optimized loop in pathTo()
- Dasher: TODO remains related to dash length issue

Hope it is good now, I plan to work soon on the Array Cache & dispose code.

Here are my comments to your emails:

1st email:
2015-08-13 22:32 GMT+02:00 Jim Graham <james.graham at oracle.com>:

>
> On 8/10/2015 2:15 PM, Laurent Bourgès wrote:
>
>> Jim,
>>
>> Here is the new webrev including your proposals to use 32.31 fixed-point
>> maths and double-precision in addLine() as it is definitely better:
>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-s3.3/
>>
>
> Why are ceil_int and floor_int implemented differently?
>

Fixed and tested OK.

>
> For CHECK_NAN, how fast is Math.isNaN?  Is it worth first comparing the
> intpart to the value that NaN converts to before calling isNaN?  The
> majority case, by a wide margin, is that the value is not NaN.
>

That's the aim: I test first the most probable case (a <= intpart) for
ceil() and then check possible overflow and finally check for NaN values to
avoid adding / substracting 1 !
I made benchmarks and this implementation optimized for the integer domain
works very well: 2x faster.

>
> Helpers.java, line 214 - why move the declaration into the for loop? That
> is confusing to me since "j" is not used in the for() conditions.
>

Reverted, sorry.

> MarlinConst - It looks like the subpixel sizes are now properties.  Did
> that have performance consequences?  The values are no longer true literal
> constants when compiling the code now.
>

In Marlin, several constant values are customizable by system properties
for some time ! However, hotspot considers them as "proper" constants and
performs its optimizations at runtime to final static fields => inlined as
fast as true literals.

2nd email:

> 2015-08-13 22:35 GMT+02:00 Jim Graham <james.graham at oracle.com>:
>
> On 8/10/2015 2:15 PM, Laurent Bourgès wrote:
>
>>     Renderer.java, line 792: Do you need to copy ERR_STEP_MAX into a
>>     local?  It is a statically initialized final int - that should
>>     already be a constant that gets copied into the code.
>>
>>
>> I always copy constants used in loops to avoid repeated constant lookups.
>>
>
> I was under the understanding that if a constant is declared final and
> static, then it is compiled directly into the code and there is no constant
> lookup.  Check the bytecodes and you should see a hard-coded literal
> 0x7fffffff everywhere it is used...
>

I agree but it seems slightly faster: probably loading the literal is a bit
slower than loading from the stack ?
Maybe some hotspot experts could give their opinion.
As this is the renderer hot loop, I prefer making it as fast as possible.

3rd email:
2015-08-13 22:45 GMT+02:00 Jim Graham <james.graham at oracle.com>:

> Is multiplying by POWER_2_TO_32 really faster than scalb?  Quick power of
> 2 multiplies was supposed to be what scalb was good at, but if they didn't
> make it a Hotspot intrinsic then it may not work as well...
>

I do not know if there is an intrinsic implementation but it is 2x faster
in my microbenchmarks. I looked at Math.scalb() implementation and it has
several conditional checks (huge exponents handled in while loop) and it
computes 2^n at every call:
that's why I precompute the correct scaling factor once !

>
> Renderer.java, line 807 - missing space before =
>

Fixed.

 4th email:
2015-08-13 22:56 GMT+02:00 Jim Graham <james.graham at oracle.com>:

> What is with the new added "default" cases?  Do they help the compiler?
>

It just fixes several Findbugs warnings without any side-effect.

> I'll defer to Phil on the naming of the new system properties
> (sun.java2d.renderer.*).  Eventually Marlin may replace both Pisces and
> Ductus and then the generic names make sense.  Should they have "marlin" in
> the name until then?
>

This is not new and present in the first Marlin patch.
However, these properties are helpful to tune Marlin settings (performance
and quality).

I could remove some properties in the future once Marlin will be tested
enough and its tuning done.

Phil's email:

2015-08-13 23:20 GMT+02:00 Phil Race <philip.race at oracle.com>:

> On 08/13/2015 01:56 PM, Jim Graham wrote:
>
>> What is with the new added "default" cases?  Do they help the compiler?
>>
>> I'll defer to Phil on the naming of the new system properties
>> (sun.java2d.renderer.*).  Eventually Marlin may replace both Pisces and
>> Ductus and then the generic names make sense.  Should they have "marlin" in
>> the name until then?
>>
>
> I think that at least for now they should either be
> sun.java2d.renderer.marlin.* or
> just sun.java2d.marlin.*
>

Ok, I can rename them in the next webrev as it requires me to upgrade all
my testing & benchmarking scripts...

> But the more important question may be around how much benefit these
> bring compared to the additional testing overhead of so many combinations ?
>

I advocate it is not easy to test all combinations but it seems not a
reasonable testing objective.

As I said, it is mainly tuning or quality settings but only few are
important (subpixels X / Y, tile size, initial pixel size).

Jim, thanks for all your efforts & take care,

Best regards,

Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/graphics-rasterizer-dev/attachments/20150817/1cd2c939/attachment.html>