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

[Philip Race]

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
>     >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190325/1c6da7b6/attachment-0001.html>