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
<[email protected] <mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>> 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
<[email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>>
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
<[email protected] <mailto:[email protected]>
<mailto:[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
>> >
>>
>>
>>
>
>
> --
> Dmitry Batrak
> Senior Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop
>
--
Best regards, Sergey.
