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

Dmitry Batrak dmitry.batrak at jetbrains.com
Tue Mar 12 08:32:24 UTC 2019


 > 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/
Part 1 (caching hb_face_t):
    http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-1/
Part 2 (tables caching removal):
    http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-2/
Part 3 (scaler pointer passing removal):
    http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-3/

Best regards,
Dmitry Batrak

On Fri, Mar 8, 2019 at 3:21 AM Philip Race <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/>
> >
> > Best regards,
> > Dmitry Batrak
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190312/79d1048c/attachment.html>


More information about the 2d-dev mailing list