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