RFR: 8186317: Cache font layout tables for use by harfbuzz

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8186317: Cache font layout tables for use by harfbuzz

Phil Race
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.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Dmitry Batrak
Hello,

I was going to propose a related change a bit later, but thought it makes sense to let you know now, as you're working on a similar thing.

In our OpenJDK-based runtime, we've optimized HarfBuzz invocations, by creating hb_face_t object only once per Font2D instance.
This can make layout 2-3 times faster (more prominent for fonts with a lot of layout rules, e.g. Fira Code).
We have this working in production for about a year now, without issues.

I've created a corresponding webrev here
  http://cr.openjdk.java.net/~dbatrak/hb-optimization/webrev.00/
It's generated 'as is', it should probably be reworked, if it's going to be included in OpenJDK. In particular, caching of hb_face_t objects
should probably be encapsulated in SunLayoutEngine, and not done in Font2D object directly.

I'm ready to work on the change further, following the formal procedure (raising the ticket, submitting RFRs, etc). So, please let me know
if you're interested, and whether you have any comments or suggestions. In any case, I'll require someone sponsoring this change,
as I don't have a Committer status.

Best regards,
Dmitry Batrak


On Thu, Aug 17, 2017 at 12:07 AM, Phil Race <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Phil Race
Hi,

If you read the bug here I wrote :
>Note that harfbuzz still will likely copy and sanitise the table each time
>we create a face object but fixing that needs a deeper understanding
>(perhaps too deep) of how we can re-use harfbuzz objects across
> calls to shape. Something to consider later

So I am aware that we are taking a per-call overhead of rebuilding this info and today's fix
is a lower-cost / risk step towards reducing that.
I needed to take a closer look at the lifecycle and sharability of these structures before
doing anything but don't have time right now. Eg, it isn't clear to me if your code is thread safe.

Also I intend to remove the ICU code as obsolete and the caching code I am using in
this fix was then going to look like dead code so I wanted to make sure it was hooked up
into HB and serving a purpose before that removal.

Seems like you must have backported the HB code to JDK 8u ?
Or are you already shipping JDK 9 based OpenJDK ?


But in summary, yes, the general idea you propose is something that is "on the list".
Probably some lines of what you have in that webrev can be adopted but I don't
yet have a view on whether it should all be adopted directly.
However a similar end result is the goal ..

-phil.


On 08/18/2017 02:31 AM, Dmitry Batrak wrote:
Hello,

I was going to propose a related change a bit later, but thought it makes sense to let you know now, as you're working on a similar thing.

In our OpenJDK-based runtime, we've optimized HarfBuzz invocations, by creating hb_face_t object only once per Font2D instance.
This can make layout 2-3 times faster (more prominent for fonts with a lot of layout rules, e.g. Fira Code).
We have this working in production for about a year now, without issues.

I've created a corresponding webrev here
  http://cr.openjdk.java.net/~dbatrak/hb-optimization/webrev.00/
It's generated 'as is', it should probably be reworked, if it's going to be included in OpenJDK. In particular, caching of hb_face_t objects
should probably be encapsulated in SunLayoutEngine, and not done in Font2D object directly.

I'm ready to work on the change further, following the formal procedure (raising the ticket, submitting RFRs, etc). So, please let me know
if you're interested, and whether you have any comments or suggestions. In any case, I'll require someone sponsoring this change,
as I don't have a Committer status.

Best regards,
Dmitry Batrak


On Thu, Aug 17, 2017 at 12:07 AM, Phil Race <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Dmitry Batrak
> Seems like you must have backported the HB code to JDK 8u ?
Yes, we did.

> However a similar end result is the goal ...
Let me know if I can help this process in any way.

Best regards,
Dmitry Batrak


On Fri, Aug 18, 2017 at 9:21 PM, Phil Race <[hidden email]> wrote:
Hi,

If you read the bug here I wrote :
>Note that harfbuzz still will likely copy and sanitise the table each time
>we create a face object but fixing that needs a deeper understanding
>(perhaps too deep) of how we can re-use harfbuzz objects across
> calls to shape. Something to consider later

So I am aware that we are taking a per-call overhead of rebuilding this info and today's fix
is a lower-cost / risk step towards reducing that.
I needed to take a closer look at the lifecycle and sharability of these structures before
doing anything but don't have time right now. Eg, it isn't clear to me if your code is thread safe.

Also I intend to remove the ICU code as obsolete and the caching code I am using in
this fix was then going to look like dead code so I wanted to make sure it was hooked up
into HB and serving a purpose before that removal.

Seems like you must have backported the HB code to JDK 8u ?
Or are you already shipping JDK 9 based OpenJDK ?


But in summary, yes, the general idea you propose is something that is "on the list".
Probably some lines of what you have in that webrev can be adopted but I don't
yet have a view on whether it should all be adopted directly.
However a similar end result is the goal ..

-phil.


On 08/18/2017 02:31 AM, Dmitry Batrak wrote:
Hello,

I was going to propose a related change a bit later, but thought it makes sense to let you know now, as you're working on a similar thing.

In our OpenJDK-based runtime, we've optimized HarfBuzz invocations, by creating hb_face_t object only once per Font2D instance.
This can make layout 2-3 times faster (more prominent for fonts with a lot of layout rules, e.g. Fira Code).
We have this working in production for about a year now, without issues.

I've created a corresponding webrev here
  http://cr.openjdk.java.net/~dbatrak/hb-optimization/webrev.00/
It's generated 'as is', it should probably be reworked, if it's going to be included in OpenJDK. In particular, caching of hb_face_t objects
should probably be encapsulated in SunLayoutEngine, and not done in Font2D object directly.

I'm ready to work on the change further, following the formal procedure (raising the ticket, submitting RFRs, etc). So, please let me know
if you're interested, and whether you have any comments or suggestions. In any case, I'll require someone sponsoring this change,
as I don't have a Committer status.

Best regards,
Dmitry Batrak


On Thu, Aug 17, 2017 at 12:07 AM, Phil Race <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Phil Race
In reply to this post by Phil Race
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.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Steven R. Loomis
The change looks good to me

On Fri, 25 Aug 2017 13:16:03 -0700, Phil Race <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Prahalad kumar Narayanan
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:[hidden email]]
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 <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186317: Cache font layout tables for use by harfbuzz

Phil Race
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:[hidden email]]
> 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 <[hidden email]>
> 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

Reply | Threaded
Open this post in threaded view
|

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