[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [PATCH] 8220231: Cache HarfBuzz face object for same font's text layout calls
From: Phil Race <philip.race () oracle ! com>
Date: 2019-04-11 16:45:34
Message-ID: f3e92d7d-7df1-bdbe-5061-4eb755f3da2e () oracle ! com
[Download RAW message or body]
[Attachment #2 (text/plain)]
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@oracle.com <mailto:Sergey.Bylokhov@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@jetbrains.com <mailto:dmitry.batrak@jetbrains.com>
> <mailto:dmitry.batrak@jetbrains.com
> <mailto:dmitry.batrak@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@oracle.com <mailto:philip.race@oracle.com>
> <mailto:philip.race@oracle.com <mailto:philip.race@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@oracle.com <mailto:philip.race@oracle.com>
> <mailto:philip.race@oracle.com <mailto:philip.race@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.
>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
OK .. since no one else seems to be doing this I'll handle it for
you.<br>
<br>
-phil.<br>
<br>
<div class="moz-cite-prefix">On 4/9/19 12:28 AM, Dmitry Batrak
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAET5FPvQETTpsgcrUF+Ogc7_9=EQzZjMBBiiRfOK0OebjJWcNQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">
<div>Thanks!</div>
<div><br>
</div>
<div>Hopefully someone can push (sponsor) it now.</div>
<div><br>
</div>
<div>Best regards,</div>
<div>Dmitry Batrak<br>
</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Mon, Apr 8, 2019 at 10:16
PM Sergey Bylokhov <<a
href="mailto:Sergey.Bylokhov@oracle.com"
moz-do-not-send="true">Sergey.Bylokhov@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">+1<br>
<br>
On 01/04/2019 02:12, Alexey Ushakov wrote:<br>
> Looks good for me.<br>
> <br>
> Best Regards,<br>
> Alexey<br>
> <br>
> On Mon, Apr 1, 2019 at 10:46 AM Dmitry Batrak <<a
href="mailto:dmitry.batrak@jetbrains.com" target="_blank"
moz-do-not-send="true">dmitry.batrak@jetbrains.com</a>
<mailto:<a href="mailto:dmitry.batrak@jetbrains.com"
target="_blank" \
moz-do-not-send="true">dmitry.batrak@jetbrains.com</a>>> wrote:<br>
> <br>
> > Sorry for the delay. I've now finished
verifying this and it is a +1 from me.<br>
> Thanks!<br>
> <br>
> Anyone else, please? A second reviewer is required.<br>
> <br>
> On Mon, Mar 25, 2019 at 11:19 PM Philip Race <<a
href="mailto:philip.race@oracle.com" target="_blank"
moz-do-not-send="true">philip.race@oracle.com</a>
<mailto:<a href="mailto:philip.race@oracle.com"
target="_blank" \
moz-do-not-send="true">philip.race@oracle.com</a>>> wrote:<br>
> <br>
> Sorry for the delay. I've now finished
verifying this and it is a +1 from me.<br>
> <br>
> -phil.<br>
> <br>
> On 3/12/19, 1:32 AM, Dmitry Batrak wrote:<br>
>> > This looks good to me, if I understand
correctly that we now create<br>
>> > the face on first use and cache it fin
Java or as long as the Font2D is<br>
>> > alive.<br>
>> > And the JNIEnv is always found on<br>
>> That's correct. The assumption is that
HarfBuzz doesn't create its own threads,<br>
>> so HarfBuzz-related native code will always
be invoked from a Java thread<br>
>> (as part of 'shape' call), and so JNIEnv
will be available in that context.<br>
>><br>
>> I've updated the webrev by including a
stress test for multi-threaded behaviour<br>
>> testing. Apart from the test, webrev also
has some cosmetic differences<br>
>> from the previous version (like a comment
fix or parameter order changing),<br>
>> appeared during 'splitting' process. To
simplify the review, I'm also providing<br>
>> the links to the 'split' version of the
same webrev - three parts that produce<br>
>> the same result when combined. I've not
tested the changes separately<br>
>> (except basic compilation check).<br>
>><br>
>> Complete change:<br>
>> <a
href="http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01/</a> \
<<a
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01/</a>><br>
>> Part 1 (caching hb_face_t):<br>
>> <a
href="http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-1/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-1/</a> \
<<a
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-1/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-1/</a>><br>
>> Part 2 (tables caching removal):<br>
>> <a
href="http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-2/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-2/</a> \
<<a
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-2/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-2/</a>><br>
>> Part 3 (scaler pointer passing removal):<br>
>> <a
href="http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-3/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-3/</a> \
<<a
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-3/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-3/</a>><br>
>><br>
>> Best regards,<br>
>> Dmitry Batrak<br>
>><br>
>> On Fri, Mar 8, 2019 at 3:21 AM Philip Race
<<a href="mailto:philip.race@oracle.com" target="_blank"
moz-do-not-send="true">philip.race@oracle.com</a>
<mailto:<a href="mailto:philip.race@oracle.com"
target="_blank" \
moz-do-not-send="true">philip.race@oracle.com</a>>> wrote:<br>
>><br>
>> This looks good to me, if I understand
correctly that we now create<br>
>> the face on first use and cache it fin
Java or as long as the Font2D is<br>
>> alive.<br>
>> And the JNIEnv is always found on<br>
>><br>
>> I think you are right that we don't
need the caching of the tables since<br>
>> now the face will do it. The
unfortunate thing is that the removal of<br>
>> this code is<br>
>> well over half the changes and as such
it greatly muddied finding the<br>
>> changes<br>
>> that make the performance difference so
my review was harder and less<br>
>> certain<br>
>> because of that.<br>
>> It could have been separated into a
follow-on change I think so that it<br>
>> would have<br>
>> been easier to review the important
change.<br>
>><br>
>> The pScaler parameter looks like it is
unused these days which is why I<br>
>> expect you removed it but also not
directly relevant.<br>
>><br>
>> I have run builds + some tests - but
I'm not in a position to run more tests<br>
>> for a couple of weeks.<br>
>><br>
>> A (mild) stress test re-using the same
font from multiple threads eachmaking<br>
>> multiple calls into layout would be a
good addition here. That should<br>
>> help tell<br>
>> us if there are any MT or re-entrancy
problems. Can you provide such a<br>
>> test ?<br>
>> It will be a good thing to have
automatically run to catch any problems<br>
>> introduced later either on the Java
side or by an update to harfbuzz.<br>
>><br>
>> -phil.<br>
>><br>
>><br>
>><br>
>> On 3/6/19, 5:45 PM, Dmitry Batrak
wrote:<br>
>> > Hello,<br>
>> ><br>
>> > I'd like to submit a patch for
JDK-8220231. I'm not a Committer, so<br>
>> > I'll need someone to sponsor this
change.<br>
>> ><br>
>> > The proposed approach is used
without known issues in OpenJDK-based<br>
>> > JetBrains Runtime for almost three
years now. I've mentioned it<br>
>> > previously on this mailing list<br>
>> > (<a
href="https://mail.openjdk.java.net/pipermail/2d-dev/2017-August/008497.html"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://mail.openjdk.java.net/pipermail/2d-dev/2017-August/008497.html</a>).<br>
>> > The change has been refactored as
compared to the version mentioned<br>
>> > above (the logic has been moved to
SunLayoutEngine), and includes the<br>
>> > removal of font tables caching
(JDK-8186317). The latter, I believe,<br>
>> > becomes redundant with this fix.<br>
>> ><br>
>> > Issue: <a
href="https://bugs.openjdk.java.net/browse/JDK-8220231"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8220231</a><br> \
>> > Webrev: <a
href="http://cr.openjdk.java.net/~dbatrak/8220231/webrev.00/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.00/</a> \
<<a
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/</a>><br>
>> > <<a
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/</a>><br>
>> ><br>
>> > Best regards,<br>
>> > Dmitry Batrak<br>
>> ><br>
>><br>
>><br>
>><br>
> <br>
> <br>
> -- <br>
> Dmitry Batrak<br>
> Senior Software Developer<br>
> JetBrains<br>
> <a href="http://www.jetbrains.com" rel="noreferrer"
target="_blank" moz-do-not-send="true">http://www.jetbrains.com</a><br>
> The Drive to Develop<br>
> <br>
<br>
<br>
-- <br>
Best regards, Sergey.<br>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic