[OpenJDK 2D-Dev] [PATCH] 8220231: Cache HarfBuzz face object for same font's text layout calls
Phil Race
philip.race at oracle.com
Thu Apr 11 16:45:34 UTC 2019
OK .. since no one else seems to be doing this I'll handle it for you.
-phil.
On 4/9/19 12:28 AM, Dmitry Batrak wrote:
> 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 <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto: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/20190411/6921fc8b/attachment-0001.html>
More information about the 2d-dev
mailing list