[OpenJDK 2D-Dev] [PATCH] 8220231: Cache HarfBuzz face object for same font's text layout calls

[Sergey Bylokhov]

+1

On 01/04/2019 02:12, Alexey Ushakov wrote:
> Looks good for me.
> 
> Best Regards,
> Alexey
> 
> On Mon, Apr 1, 2019 at 10:46 AM Dmitry Batrak <dmitry.batrak at jetbrains.com <mailto:dmitry.batrak at jetbrains.com>> wrote:
> 
>      > Sorry for the delay. I've now finished verifying this and it is a +1 from me.
>     Thanks!
> 
>     Anyone else, please? A second reviewer is required.
> 
>     On Mon, Mar 25, 2019 at 11:19 PM Philip Race <philip.race at oracle.com <mailto:philip.race at oracle.com>> wrote:
> 
>         Sorry for the delay. I've now finished verifying this and it is a +1 from me.
> 
>         -phil.
> 
>         On 3/12/19, 1:32 AM, Dmitry Batrak wrote:
>>         > This looks good to me, if I understand correctly that we now create
>>         > the face on first use and cache it fin Java or as long as the Font2D is
>>         > alive.
>>         > And the JNIEnv is always found on
>>         That's correct. The assumption is that HarfBuzz doesn't create its own threads,
>>         so HarfBuzz-related native code will always be invoked from a Java thread
>>         (as part of 'shape' call), and so JNIEnv will be available in that context.
>>
>>         I've updated the webrev by including a stress test for multi-threaded behaviour
>>         testing. Apart from the test, webrev also has some cosmetic differences
>>         from the previous version (like a comment fix or parameter order changing),
>>         appeared during 'splitting' process. To simplify the review, I'm also providing
>>         the links to the 'split' version of the same webrev - three parts that produce
>>         the same result when combined. I've not tested the changes separately
>>         (except basic compilation check).
>>
>>         Complete change:
>>         http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01/ <http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01/>
>>         Part 1 (caching hb_face_t):
>>         http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-1/ <http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-1/>
>>         Part 2 (tables caching removal):
>>         http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-2/ <http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-2/>
>>         Part 3 (scaler pointer passing removal):
>>         http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-3/ <http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-3/>
>>
>>         Best regards,
>>         Dmitry Batrak
>>
>>         On Fri, Mar 8, 2019 at 3:21 AM Philip Race <philip.race at oracle.com <mailto:philip.race at oracle.com>> wrote:
>>
>>             This looks good to me, if I understand correctly that we now create
>>             the face on first use and cache it fin Java or as long as the Font2D is
>>             alive.
>>             And the JNIEnv is always found on
>>
>>             I think you are right that we don't need the caching of the tables since
>>             now the face will do it. The unfortunate thing is that the removal of
>>             this code is
>>             well over half the changes and as such it greatly muddied finding the
>>             changes
>>             that make the performance difference so my review was harder and less
>>             certain
>>             because of that.
>>             It could have been separated into a follow-on change I think so that it
>>             would have
>>             been easier to review the important change.
>>
>>             The pScaler parameter looks like it is unused these days which is why I
>>             expect you removed it but also not directly relevant.
>>
>>             I have run builds + some tests - but I'm not in a position to run more tests
>>             for a couple of weeks.
>>
>>             A (mild) stress test re-using the same font from multiple threads eachmaking
>>             multiple calls into layout would be a good addition here. That should
>>             help tell
>>             us if there are any MT or re-entrancy problems. Can you provide such a
>>             test ?
>>             It will be a good thing to have automatically run to catch any problems
>>             introduced later either on the Java side or by an update to harfbuzz.
>>
>>             -phil.
>>
>>
>>
>>             On 3/6/19, 5:45 PM, Dmitry Batrak wrote:
>>             > Hello,
>>             >
>>             > I'd like to submit a patch for JDK-8220231. I'm not a Committer, so
>>             > I'll need someone to sponsor this change.
>>             >
>>             > The proposed approach is used without known issues in OpenJDK-based
>>             > JetBrains Runtime for almost three years now. I've mentioned it
>>             > previously on this mailing list
>>             > (https://mail.openjdk.java.net/pipermail/2d-dev/2017-August/008497.html).
>>             > The change has been refactored as compared to the version mentioned
>>             > above (the logic has been moved to SunLayoutEngine), and includes the
>>             > removal of font tables caching (JDK-8186317). The latter, I believe,
>>             > becomes redundant with this fix.
>>             >
>>             > Issue: https://bugs.openjdk.java.net/browse/JDK-8220231
>>             > Webrev: http://cr.openjdk.java.net/~dbatrak/8220231/webrev.00/ <http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/>
>>             > <http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/>
>>             >
>>             > Best regards,
>>             > Dmitry Batrak
>>             >
>>
>>
>>
> 
> 
>     -- 
>     Dmitry Batrak
>     Senior Software Developer
>     JetBrains
>     http://www.jetbrains.com
>     The Drive to Develop
> 


-- 
Best regards, Sergey.