Re: [OpenJDK 2D-Dev] RFR: 8189809: Large performance regression in Swing text layout.

[Sergey Bylokhov]

Looks fine.

On 11/12/2017 14:39, Phil Race wrote:
Yes, it should  be negative since we want to use this as the y origin of 
the rectangle

relative to the baseline. I am using it correctly in the height calculation.

40 float height = ascent + descent + leading;
541 return new Rectangle2D.Float(0f, ascent, width, height);


http://cr.openjdk.java.net/~prr/8189809.1/

-phil.

On 12/11/2017 02:12 PM, Sergey Bylokhov wrote:

Hi, Phil.
The new code will calculate the logical bounds this way:
    541 new Rectangle2D.Float(0f, ascent, width, height);
But previously it was:
    2613 new Rectangle2D.Float(0, -tl.getAscent(), tl.getAdvance(),
    2614   tl.getAscent() + tl.getDescent() +
    2615   tl.getLeading());

Note that the second parameter is negative, is it intentionally changed?


On 07/12/2017 12:54, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8189809
webrev: http://cr.openjdk.java.net/~prr/8189809/

This partially addresses a slow-down in Swing.

Swing now usually calls Font.getStringBounds() instead of 
FontMetrics().stringWidth for text measurement.


The latter has a fast-path for latin-only simple text.

The former does not .. and at a minimum uses GlyphVector.

This fix provides a similar fast path and for direct calls to these 
methods (micro benchmarks)
that is latin text they are now very similar .. at least for typical 
strings.


In this fix as well as making this change in the font code, I updated 
Swing to more

be a little more efficient.

Before this fix I always saw Swing  coming in with a char[] .. then 
constructing a String from it.
Then it passes it to the font measurement code, which then decomposes 
the string back into a char[]
If Swing directly uses the API that takes a char[] then we save some 
overhead.



It does not get back all of the loss by Swing since

1) Swing has other overhead elsewhere - it seems

2) Swing is making 90% of these calls for a single char.
This could be from where Swing naively tries to add up char advances 
itself to

get a string advance.

 We may have to come back to that later. Perhaps with new public 
API.


-phil.










--
Best regards, Sergey.

Re: [OpenJDK 2D-Dev] RFR: 8189809: Large performance regression in Swing text layout.

[Phil Race]

Yes, it should  be negative since we want to use this as the y origin of 
the rectangle

relative to the baseline. I am using it correctly in the height calculation.

40 float height = ascent + descent + leading;
541 return new Rectangle2D.Float(0f, ascent, width, height);


http://cr.openjdk.java.net/~prr/8189809.1/

-phil.

On 12/11/2017 02:12 PM, Sergey Bylokhov wrote:

Hi, Phil.
The new code will calculate the logical bounds this way:
541 new Rectangle2D.Float(0f, ascent, width, height);
But previously it was:
2613 new Rectangle2D.Float(0, -tl.getAscent(), tl.getAdvance(),
2614   tl.getAscent() + tl.getDescent() +
2615   tl.getLeading());

Note that the second parameter is negative, is it intentionally changed?


On 07/12/2017 12:54, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8189809
webrev: http://cr.openjdk.java.net/~prr/8189809/

This partially addresses a slow-down in Swing.

Swing now usually calls Font.getStringBounds() instead of 
FontMetrics().stringWidth for text measurement.


The latter has a fast-path for latin-only simple text.

The former does not .. and at a minimum uses GlyphVector.

This fix provides a similar fast path and for direct calls to these 
methods (micro benchmarks)
that is latin text they are now very similar .. at least for typical 
strings.


In this fix as well as making this change in the font code, I updated 
Swing to more

be a little more efficient.

Before this fix I always saw Swing  coming in with a char[] .. then 
constructing a String from it.
Then it passes it to the font measurement code, which then decomposes 
the string back into a char[]
If Swing directly uses the API that takes a char[] then we save some 
overhead.



It does not get back all of the loss by Swing since

1) Swing has other overhead elsewhere - it seems

2) Swing is making 90% of these calls for a single char.
This could be from where Swing naively tries to add up char advances 
itself to

get a string advance.

 We may have to come back to that later. Perhaps with new public 
API.


-phil.

Re: [OpenJDK 2D-Dev] RFR: 8189809: Large performance regression in Swing text layout.

[Sergey Bylokhov]

Hi, Phil.
The new code will calculate the logical bounds this way:
541 new Rectangle2D.Float(0f, ascent, width, height);
But previously it was:
2613 new Rectangle2D.Float(0, -tl.getAscent(),   tl.getAdvance(),
2614   tl.getAscent() + tl.getDescent() +
2615   tl.getLeading());

Note that the second parameter is negative, is it intentionally changed?


On 07/12/2017 12:54, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8189809
webrev: http://cr.openjdk.java.net/~prr/8189809/

This partially addresses a slow-down in Swing.

Swing now usually calls Font.getStringBounds() instead of 
FontMetrics().stringWidth for text measurement.


The latter has a fast-path for latin-only simple text.

The former does not .. and at a minimum uses GlyphVector.

This fix provides a similar fast path and for direct calls to these 
methods (micro benchmarks)
that is latin text they are now very similar .. at least for typical 
strings.


In this fix as well as making this change in the font code, I updated 
Swing to more

be a little more efficient.

Before this fix I always saw Swing  coming in with a char[] .. then 
constructing a String from it.
Then it passes it to the font measurement code, which then decomposes 
the string back into a char[]
If Swing directly uses the API that takes a char[] then we save some 
overhead.



It does not get back all of the loss by Swing since

1) Swing has other overhead elsewhere - it seems

2) Swing is making 90% of these calls for a single char.
This could be from where Swing naively tries to add up char advances 
itself to

get a string advance.

     We may have to come back to that later. Perhaps with new public API.

-phil.





--
Best regards, Sergey.

Re: [OpenJDK 2D-Dev] [10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

[Laurent Bourgès]

Hi,

I pushed in jdk forrest:
http://hg.openjdk.java.net/jdk/client/rev/fd7fbc929001

Thanks for your reviews, Phil & Sergey !

Laurent


2017-12-07 20:43 GMT+01:00 Sergey Bylokhov :

> I am not an expert here, I just look to the changes and run the closed
> tests for jdk_desktop group. No new issues were found, so it looks fine to
> me.
>
> On 05/12/2017 06:30, Laurent Bourgès wrote:
>
>> Hi again,
>>
>> Here is a new webrev fixing the ClipShapeTest:
>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-082-8191814.1/
>>
>> Changes:
>> - @run twice to test both Marlin variants (float / double)
>> - use Logger to get & check marlin system properties
>> ("sun.java2d.renderer.clip.runtime.enable") to ensure the test is run
>> against a Marlin renderer having the clipping features
>>
>> I tested the fixed test on JDK9 and it fails:
>> runner finished test: sun/java2d/marlin/ClipShapeTest.java
>> Failed. Execution failed: `main' threw exception:
>> java.lang.RuntimeException: Marlin clipping not enabled at runtime !
>>
>> On JDK10 + patch, it passes:
>> sun/java2d/marlin/ClipShapeTest.java
>> Passed. Execution successful
>>
>>
>> PS: Could anyone review the patch ? before RDP1 deadline ?
>>
>> Regards,
>> Laurent
>>
>>
>> 2017-12-04 23:35 GMT+01:00 Laurent Bourgès > >:
>>
>> Hi Sergey,
>>
>> Thanks for your comment.
>>
>> This new test only validates the new clipping algorithms ie it
>> compares the rendering outputs with / without clipping enabled.
>>
>> As such algorithms are only available in Marlin 0.8.2 and the test
>> uses new system properties to enable/disable clipping, I confirm it
>> passes before (jdk9 or jdk10 before patch).
>>
>> To ensure it detects any regression, I manually inserted some bugs
>> in the clipping code, and the test failed.
>>
>> Note: I should add another test @run to check the float variant too
>> (and not only the double variant, the default in jdk10).
>>
>> Finally I could write a new performance test that would prove
>> clipping is more efficient than before.
>> Such test would fail before patch (timeout ?), but it is difficult
>> to make it robust as it depends on the hw.
>> Jim wrote a basic test in the jfx bug showing 300ms without but 2ms
>> now => gain is high.
>> A possible success condition would be: clipping gain > 10 or 50.
>>
>> Regards,
>> Laurent
>>
>>
>> Le 4 déc. 2017 11:11 PM, "Sergey Bylokhov"
>> > a
>> écrit :
>>
>> Hi, Laurent.
>>
>> On 29/11/2017 14:30, Laurent Bourgès wrote:
>>
>> - added new ClipShapeTest (jtreg) that checks all possible
>> combinations of (cap / join) for random polyline (Stroker)
>> and polygons (Filler) comparing image outputs rendered with
>> clipping enabled vs disabled
>>
>>
>> I have only one note that the test is passed before the fix, so
>> if we will regress at some point later we will not catch this.
>
>

Re: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

[Phil Race]

The explanation sounds reasonable, although I'd like to give Clemens a 
chance to review this.


-phil.

On 12/07/2017 07:16 AM, Jayathirth D V wrote:


Hello All,

Please review the following fix in JDK10 :

Bug : https://bugs.openjdk.java.net/browse/JDK-8176795

Webrev : http://cr.openjdk.java.net/~jdv/8176795/webrev.00/ 



Issue : When we draw translucent color over an opaque color in Unix 
from JDK 8 we get different color after composition compared to any 
other platform.


Root cause : From JDK 8, X Rendering extension is enabled in Unix and 
we see this problem only when we use XRender in Unix if we use GLX or 
 X11 we don’t see any issue.  Also X Rendering extension expects 
pre-multiplied alpha color values, so we need to convert the 
non-premultiplied alpha color values to pre-multiplied alpha color 
values before give pixel value to XRender for drawing. The main 
problem is we do this operation of conversion twice:


1)When we call Graphics2D.setColor() it uses ArgbPre PixelConverter 
based on SurfaceData(here it is XRenderPixMap ArgbPre surface) and 
converts the color to pre-multiplied values.


2)When we call Graphics2D.fillRect() internally before we compose the 
destination(opaque color) and source(translucent color) we prepare 
source render rectangle for X Render extension. Here again we convert 
the already converted color values to premultiplied values.


Solution : There is no need for us to do non-pre to pre color 
conversion again at XRender level so it should be removed. Also this 
logic of non-pre to pre color conversion at XRender was only used when 
source is Solid color and not Texture/Gradient. So I have completely 
removed this logic itself as it not needed anywhere else.


Thanks,

Jay

Re: [OpenJDK 2D-Dev] RFR JDK-8184429: Path clipper added in Marlin2D & MarlinFX 0.8.0

[Kevin Rushforth]

I started looking the FX patch on Friday. I hope to finish my review today.

-- Kevin


Philip Race wrote:

Yes, I approved the 2D patch. I have not yet looked at the FX version.

-phil

On 12/10/17, 12:39 PM, Laurent Bourgès wrote:

Phil,

Thanks for your review.

I suppose you approved the Java2D patch for the RFR thread: [10] RFR 
JDK-8191814:

http://mail.openjdk.java.net/pipermail/2d-dev/2017-November/008741.html

I will push tomorrow to jdk forrest as phil & sergey? approved 2D 
changes.



This RFR thread remains open for the JavaFX patch that Kevin started 
to test & review.


Cheers,
Laurent

2017-12-10 18:36 GMT+01:00 Philip Race >:


I'm giving this an OK.
I've looked at the code, if not the maths, and run our regression
test suite.
I had a bit of trouble with my full baseline run against which to
compare so I've
re-run just the test failures that seem like they might go
anywhere need this code.
and they were pre-existing.

-phil.


On 11/16/17, 3:01 PM, Laurent Bourgès wrote:

Phil,

Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-082-8184429.1/


It fixes the ClipShapeTest to run ~35s (< 2mins) on my latop: 22
test setups only (5000 random polygons each) that covers all
aspects of the new clipping algorithm.

I wonder if I should remove the 'slow' mode that has till @ignore:
both tests are ignored if I run jtreg -ignore:quiet although I
would like to only ignore the @ignore run (1/2).


Once again, I have to create a new JBS bug for Marlin2D tomorrow
then start a new RFR with a complete change log.

Laurent

2017-11-16 10:50 GMT+01:00 Laurent Bourgès
>:

Sergey,

You can generate a number of images using a few threads
when the flag is on then switch it off and compares
results. The test should not draw different(on/off)
modes in parallel but it can draw the images for the
same mode.


Yes that is always possible but I do not want to spend too
much time improving the test to parallelize it.

I fixed it last night and it runs only 22 test setups (1
stroke width = 8px, no dashes) and takes ~ 30 seconds on my
laptop (single-thread).
I verified that any bug in Marlin clipping is detected by
this new variant of the tests.
Will provide a new patch asap.
 



Finally I will minimize the number of stroker tests:
use only 1 stroke width (=5px) and that should be
enough to stay below timeout (2mins).


Note that the test can be run on some slow/virtual
systems, it would be good to have some additional time.

Is there any documentation about jtreg tags ?


You can find it here:
http://openjdk.java.net/jtreg/tag-spec.html



Thanks.

Laurent




-- 
-- 
Laurent Bourgès





--
--
Laurent Bourgès

Re: [OpenJDK 2D-Dev] RFR JDK-8184429: Path clipper added in Marlin2D & MarlinFX 0.8.0

[Philip Race]

Yes, I approved the 2D patch. I have not yet looked at the FX version.

-phil

On 12/10/17, 12:39 PM, Laurent Bourgès wrote:

Phil,

Thanks for your review.

I suppose you approved the Java2D patch for the RFR thread: [10] RFR 
JDK-8191814:

http://mail.openjdk.java.net/pipermail/2d-dev/2017-November/008741.html

I will push tomorrow to jdk forrest as phil & sergey? approved 2D changes.


This RFR thread remains open for the JavaFX patch that Kevin started 
to test & review.


Cheers,
Laurent

2017-12-10 18:36 GMT+01:00 Philip Race >:


I'm giving this an OK.
I've looked at the code, if not the maths, and run our regression
test suite.
I had a bit of trouble with my full baseline run against which to
compare so I've
re-run just the test failures that seem like they might go
anywhere need this code.
and they were pre-existing.

-phil.


On 11/16/17, 3:01 PM, Laurent Bourgès wrote:

Phil,

Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-082-8184429.1/


It fixes the ClipShapeTest to run ~35s (< 2mins) on my latop: 22
test setups only (5000 random polygons each) that covers all
aspects of the new clipping algorithm.

I wonder if I should remove the 'slow' mode that has till @ignore:
both tests are ignored if I run jtreg -ignore:quiet although I
would like to only ignore the @ignore run (1/2).


Once again, I have to create a new JBS bug for Marlin2D tomorrow
then start a new RFR with a complete change log.

Laurent

2017-11-16 10:50 GMT+01:00 Laurent Bourgès
>:

Sergey,

You can generate a number of images using a few threads
when the flag is on then switch it off and compares
results. The test should not draw different(on/off) modes
in parallel but it can draw the images for the same mode.


Yes that is always possible but I do not want to spend too
much time improving the test to parallelize it.

I fixed it last night and it runs only 22 test setups (1
stroke width = 8px, no dashes) and takes ~ 30 seconds on my
laptop (single-thread).
I verified that any bug in Marlin clipping is detected by
this new variant of the tests.
Will provide a new patch asap.


Finally I will minimize the number of stroker tests:
use only 1 stroke width (=5px) and that should be
enough to stay below timeout (2mins).


Note that the test can be run on some slow/virtual
systems, it would be good to have some additional time.

Is there any documentation about jtreg tags ?


You can find it here:
http://openjdk.java.net/jtreg/tag-spec.html



Thanks.

Laurent




-- 
-- 
Laurent Bourgès





--
--
Laurent Bourgès

Re: [OpenJDK 2D-Dev] RFR: 8189809: Large performance regression in Swing text layout.

[Prahalad Kumar Narayanan]

Hello Phil

The change looks good to me.

I found two interesting changes in your webrev. 
. The move to use FontDesignMetrics to compute width should be better than 
generating GlyphVector for the same.
. Besides, the double conversion from chars[] to String and vice versa has been 
avoided as well.

Thank you
Have a good day

Prahalad

-Original Message-
From: Phil Race 
Sent: Friday, December 08, 2017 2:24 AM
To: 2d-dev; swing-...@openjdk.java.net
Subject: [OpenJDK 2D-Dev] RFR: 8189809: Large performance regression in Swing 
text layout.

Bug: https://bugs.openjdk.java.net/browse/JDK-8189809
webrev: http://cr.openjdk.java.net/~prr/8189809/

This partially addresses a slow-down in Swing.

Swing now usually calls Font.getStringBounds() instead of 
FontMetrics().stringWidth for text measurement.

The latter has a fast-path for latin-only simple text.

The former does not .. and at a minimum uses GlyphVector.

This fix provides a similar fast path and for direct calls to these methods 
(micro benchmarks) that is latin text they are now very similar .. at least for 
typical strings.

In this fix as well as making this change in the font code, I updated Swing to 
more be a little more efficient.

Before this fix I always saw Swing  coming in with a char[] .. then 
constructing a String from it.
Then it passes it to the font measurement code, which then decomposes the 
string back into a char[] If Swing directly uses the API that takes a char[] 
then we save some overhead.


It does not get back all of the loss by Swing since

1) Swing has other overhead elsewhere - it seems

2) Swing is making 90% of these calls for a single char.
This could be from where Swing naively tries to add up char advances itself to 
get a string advance.

 We may have to come back to that later. Perhaps with new public API.

-phil.

Re: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting translucent colors on volatile images using XRender.

[Jayathirth D V]

Hello All,

 

Gentle reminder for review.

 

Thanks,

Jay

 

From: Jayathirth D V 
Sent: Thursday, December 07, 2017 8:47 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8176795: Wrong color drawn when painting 
translucent colors on volatile images using XRender.

 

Hello All,

 

Please review the following fix in JDK10 :

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8176795 

Webrev : http://cr.openjdk.java.net/~jdv/8176795/webrev.00/ 

 

Issue : When we draw translucent color over an opaque color in Unix from JDK 8 
we get different color after composition compared to any other platform.

 

Root cause : From JDK 8, X Rendering extension is enabled in Unix and we see 
this problem only when we use XRender in Unix if we use GLX or  X11 we don't 
see any issue.  Also X Rendering extension expects pre-multiplied alpha color 
values, so we need to convert the non-premultiplied alpha color values to 
pre-multiplied alpha color values before give pixel value to XRender for 
drawing. The main problem is we do this operation of conversion twice:

1)  When we call Graphics2D.setColor() it uses ArgbPre PixelConverter based 
on SurfaceData(here it is XRenderPixMap ArgbPre surface) and converts the color 
to pre-multiplied values.

2)  When we call Graphics2D.fillRect() internally before we compose the 
destination(opaque color) and source(translucent color) we prepare source 
render rectangle for X Render extension. Here again we convert the already 
converted color values to premultiplied values. 

 

Solution : There is no need for us to do non-pre to pre color conversion again 
at XRender level so it should be removed. Also this logic of non-pre to pre 
color conversion at XRender was only used when source is Solid color and not 
Texture/Gradient. So I have completely removed this logic itself as it not 
needed anywhere else.

 

Thanks,

Jay