RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout

[Per Minborg]

On Tue, 29 Aug 2023 22:04:40 GMT, Phil Race <prr at openjdk.org> wrote:

> 8318364: Add an FFM-based implementation of harfbuzz OpenType layout

src/java.desktop/share/classes/sun/font/HBShaper.java line 55:

> 53: public class HBShaper {
> 54: 
> 55:     /*

Nice with the original C struct as a comment.

src/java.desktop/share/classes/sun/font/HBShaper.java line 65:

> 63:      * };
> 64:      */
> 65:    private static final UnionLayout VarIntLayout = MemoryLayout.unionLayout(

I was a bit confused by the naming. Suggest `VarIntLayout` -> `VAR_INT_LAYOUT`

src/java.desktop/share/classes/sun/font/HBShaper.java line 108:

> 106:     ).withName("hb_glyph_info_t");
> 107: 
> 108:     private static VarHandle getVarHandle(MemoryLayout layout, String name) {

This method could take a `SequenceLayout` instead of a `MemoryLayout` as it only works for SeequenceLayouts.

src/java.desktop/share/classes/sun/font/HBShaper.java line 129:

> 127:     private static MethodHandle jdk_hb_shape_handle;
> 128: 
> 129:     private static FunctionDescriptor get_nominal_glyph_fd;

All the `*_fd` variables could be converted into local variables.

src/java.desktop/share/classes/sun/font/HBShaper.java line 130:

> 128: 
> 129:     private static FunctionDescriptor get_nominal_glyph_fd;
> 130:     private static MethodHandle get_nominal_glyph_mh;

Declaring all these `final` would improve performance.  For example

`private static final FunctionDescription GET_NOMINAL_GLYPH_MH;`

src/java.desktop/share/classes/sun/font/HBShaper.java line 314:

> 312:         Font2D font2D = scopedFont2D.get();
> 313:         int glyphID = font2D.charToGlyph(unicode);
> 314:         MemorySegment glyphIDPtr = glyph.reinterpret(4);

As a general comment, it is better to use slicing rather than reinterpret.

src/java.desktop/share/classes/sun/font/HBShaper.java line 405:

> 403:     private static final ScopedValue<FontStrike> scopedFontStrike = ScopedValue.newInstance();
> 404:     private static final ScopedValue<GVData> scopedGVData = ScopedValue.newInstance();
> 405:     private static final ScopedValue<Point2D.Float> scopedStartPt = ScopedValue.newInstance();

Using only one ScopedValue and storing a `record(Font2D font2D, FontStrike fontStrike, GVData gvData, Point2D.float point2d) {}` of the various objects will provide much better performance.

src/java.desktop/share/classes/sun/font/HBShaper.java line 428:

> 426:          * The alternative of creating bound method handles is far too slow.
> 427:          */ 
> 428:         ScopedValue.where(scopedFont2D, font2D)

I think a static `ConcurrentHashMap<Long, Record>` would provide better performance. We could clean up the key when the value is used.  Use the Thread.threadId() as the key.

src/java.desktop/share/classes/sun/font/HBShaper.java line 468:

> 466:          * done with it.
> 467:          */
> 468:         MemorySegment data_ptr = data_ptr_out.reinterpret(ADDRESS.byteSize());

Suggest using `.asSlice()` here as it is an unrestricted and safer method.

src/java.desktop/share/classes/sun/font/HBShaper.java line 474:

> 472:         }
> 473:         byte[] data = font2D.getTableBytes(tag);
> 474:         if (data == null) {

No setting data_ptr_out to NULL here?

src/java.desktop/share/classes/sun/font/HBShaper.java line 524:

> 522:         }
> 523:  
> 524:         private synchronized MemorySegment getFace() {

We are already synchronized via the `faceMap`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309776173
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309775469
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309770631
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309774045
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309772610
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309813499
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1310541406
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1310545668
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309793358
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309791413
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1309810009