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

[Dmitry Batrak]

A second reviewer is still required.

Best regards,
Dmitry Batrak.

On Mon, Apr 1, 2019 at 10:44 AM Dmitry Batrak <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>
> 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/
>> Part 1 (caching hb_face_t):
>>     http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-1/
>> Part 2 (tables caching removal):
>>     http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-2/
>> Part 3 (scaler pointer passing removal):
>>     http://cr.openjdk.java.net/~dbatrak/8220231/webrev.01-3/
>>
>> Best regards,
>> Dmitry Batrak
>>
>> On Fri, Mar 8, 2019 at 3:21 AM Philip Race <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/>
>>> >
>>> > Best regards,
>>> > Dmitry Batrak
>>> >
>>>
>>
>>
>>
>
> --
> Dmitry Batrak
> Senior Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190408/b4267168/attachment.html>