[OpenJDK 2D-Dev] [PATCH] 8220231: Cache HarfBuzz face object for same font's text layout calls
Dmitry Batrak
dmitry.batrak at jetbrains.com
Tue Apr 9 07:28:26 UTC 2019
Thanks!
Hopefully someone can push (sponsor) it now.
Best regards,
Dmitry Batrak
On Mon, Apr 8, 2019 at 10:16 PM Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
wrote:
> +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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190409/bcb090e1/attachment-0001.html>
More information about the 2d-dev
mailing list