On Tue, 29 Aug 2023 22:04:40 GMT, Phil Race <p...@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
