Looks good for me. Best Regards, Alexey
On Mon, Apr 1, 2019 at 10:46 AM Dmitry Batrak <[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]> > 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 >
