[OpenJDK 2D-Dev] RFR: 8186317: Cache font layout tables for use by harfbuzz

[Prahalad Kumar Narayanan]

Hello Phil

You are correct. I missed to take note of calloc() at Line 347.
The changes are good.

Thank you
Have a good day

Prahalad N.

-----Original Message-----
From: Phil Race 
Sent: Monday, August 28, 2017 11:34 PM
To: Prahalad Kumar Narayanan; srl
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR: 8186317: Cache font layout tables for use by harfbuzz

You elided an important line here :

  347   TTLayoutTableCache* ltc = calloc(1, sizeof(TTLayoutTableCache));


The memory is allocated using calloc so is zeroed on allocation.

-phil.

On 08/26/2017 12:06 PM, Prahalad Kumar Narayanan wrote:
> 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