[OpenJDK 2D-Dev] [PATCH] 8220231: Cache HarfBuzz face object for same font's text layout calls

[Philip Race]

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/>
>
> Best regards,
> Dmitry Batrak
>