[OpenJDK 2D-Dev] RFR: 8186317: Cache font layout tables for use by harfbuzz
Prahalad Kumar Narayanan
prahalad.kumar.narayanan at oracle.com
Sat Aug 26 19:06:09 UTC 2017
Hello Phil
The change looks good to me.
Thank you for the observation in performance; It validates the fix.
Just a minor observation in code which isn't a part of this fix.
When we initialize TTLayoutTableCacheEntry within newLayoutTableCache(), we are not setting its pointer to NULL.
File: sunFont.c
346 JNIEXPORT TTLayoutTableCache* newLayoutTableCache() {
...
350 for(i=0;i<LAYOUTCACHE_ENTRIES;i++) {
351 ltc->entries[i].len = -1; <-- Missing initialization of ltc->entries[i].ptr = NULL here.
352 }
Though the code changes in this fix require ltc->entries[i].ptr, the safety check for entries[i].len != -1 prevents the usage.
Hence, the problem might have been evaded.
File: hb-jdk-font.cc
300 if (jdkFontInfo->layoutTables->entries[cacheIdx].len != -1) { <-- safety check prevents garbage value
301 length = jdkFontInfo->layoutTables->entries[cacheIdx].len;
302 buffer = (void*)jdkFontInfo->layoutTables->entries[cacheIdx].ptr;
303 }
...
306 if (buffer == NULL) {
But the code to free layout table cache does not check whether the buffer length is -1.
It checks on the state of the ptr variable.
File: sunFont.c
364 JNIEXPORT void freeLayoutTableCache(TTLayoutTableCache* ltc) {
365 if (ltc) {
366 int i;
367 for(i=0;i<LAYOUTCACHE_ENTRIES;i++) {
368 if(ltc->entries[i].ptr) free (ltc->entries[i].ptr); <-- A garbage value for .ptr could trigger a crash.
Thank you
Have a good day
Prahalad N.
-----Original Message-----
From: srl [mailto:srl at icu-project.org]
Sent: Saturday, August 26, 2017 4:28 AM
To: Phil Race
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR: 8186317: Cache font layout tables for use by harfbuzz
The change looks good to me
On Fri, 25 Aug 2017 13:16:03 -0700, Phil Race <philip.race at oracle.com>
wrote:
> Dmitry commented on what JetBrains are doing but do I have any takers
> to actually review this change ?
>
> -phil.
>
> On 08/16/2017 02:07 PM, Phil Race wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186317
>> Webrev: http://cr.openjdk.java.net/~prr/8186317/
>>
>> In the ICU code path, we cache the layout tables in native memory so
that
>> when requested by ICU we can just hand the pointer, saving the
>> overhead of copying the Java array for every call to layout.
>>
>> We weren't doing this for harfbuzz and this webrev fixes that.
>>
>> I noticed harfbuzz always retrieves the 'head' table so I decided to
>> add that to the list of cached tables.
>>
>> The overall result is something like 25-30% performance gain.
>>
>> -phil.
--
--
Sent via POST HTTP/1.1
More information about the 2d-dev
mailing list