Thanks! Hopefully someone can push (sponsor) it now.
Best regards, Dmitry Batrak On Mon, Apr 8, 2019 at 10:16 PM Sergey Bylokhov <sergey.bylok...@oracle.com> 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 < > dmitry.bat...@jetbrains.com <mailto:dmitry.bat...@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.r...@oracle.com > <mailto:philip.r...@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/ < > 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.r...@oracle.com <mailto:philip.r...@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 > >> > > >> > >> > >> > > > > > > -- > > Dmitry Batrak > > Senior Software Developer > > JetBrains > > http://www.jetbrains.com > > The Drive to Develop > > > > > -- > Best regards, Sergey. >
