[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: Philip Race <philip.race () oracle ! com>
Date: 2019-03-25 20:19:11
Message-ID: 5C9937BF.9010500 () oracle ! com
[Download RAW message or body]
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>> 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
> >
>
>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
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:
<blockquote
cite="mid:CAET5FPsfgaJJNNwQtrmopBeQss8NOjh-tAm93ULoc1wrTDT-og@mail.gmail.com"
type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
> 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>
</div>
<div>That's correct. The assumption is that HarfBuzz
doesn't create its own threads,</div>
<div>so HarfBuzz-related native code will always be
invoked from a Java thread</div>
<div>(as part of 'shape' call), and so JNIEnv will be
available in that context.</div>
<div><br>
</div>
<div>I've updated the webrev by including a stress
test for multi-threaded behaviour</div>
<div>testing. Apart from the test, webrev also has
some cosmetic differences</div>
<div>from the previous version (like a comment fix or
parameter order changing),</div>
<div>appeared during 'splitting' process. To simplify
the review, I'm also providing</div>
<div>the links to the 'split' version of the same
webrev - three parts that produce</div>
<div>the same result when combined. I've not tested
the changes separately</div>
<div>(except basic compilation check).</div>
<div><br>
</div>
<div>Complete change:</div>
<div> <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01/">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01/</a></div>
<div>Part 1 (caching hb_face_t):</div>
<div> <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-1/">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-1/</a></div>
<div>Part 2 (tables caching removal):</div>
<div> <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-2/">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-2/</a></div>
<div>Part 3 (scaler pointer passing removal):</div>
<div> <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.01-3/">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-3/</a><br>
</div>
</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">
<div>Best regards,</div>
<div>Dmitry Batrak</div>
<div><br>
</div>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Mar 8,
2019 at 3:21 AM Philip Race <<a
moz-do-not-send="true"
\
href="mailto:philip.race@oracle.com">philip.race@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">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 moz-do-not-send="true"
href="https://mail.openjdk.java.net/pipermail/2d-dev/2017-August/008497.html"
rel="noreferrer" \
target="_blank">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 moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8220231"
rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8220231</a><br> > \
Webrev: <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/"
rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~dbatrak/8220231/webrev.00/</a> <br>
> <<a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/"
rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Edbatrak/8220231/webrev.00/</a>><br> \
><br> > Best regards,<br>
> Dmitry Batrak<br>
><br>
</blockquote>
</div>
<br clear="all">
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic