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 <[email protected]
<mailto:[email protected]>> 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
>
