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:[email protected]]
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 <[email protected]>
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
