On Tue, 16 May 2023 13:53:32 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
> As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java line 155: > 153: assert isValidCarrier(carrier); > 154: assert carrier != MemorySegment.class > 155: // MemorySegment byteSize must always equal > ADDRESS_SIZE_BITS Suggestion: // MemorySegment byteSize must always equal ADDRESS_SIZE_BYTES test/jdk/java/foreign/TestIllegalLink.java line 122: > 120: { > 121: > FunctionDescriptor.ofVoid(C_INT.withByteAlignment(2)), > 122: "Layout bit alignment must be natural alignment" Suggestion: "Layout byte alignment must be natural alignment" And for all the other occurrences. test/jdk/java/foreign/TestLayoutPaths.java line 49: > 47: public class TestLayoutPaths { > 48: > 49: @Test(expectedExceptions = IllegalArgumentException.class) Remove this test, since its is the same as the next one. test/jdk/java/foreign/TestLayoutPaths.java line 185: > 183: for (int i = 0 ; i < 4 ; i++) { > 184: long bitOffset = g.byteOffset(groupSelector.apply(i)); > 185: assertEquals(offsets[i], bitOffset); Suggestion: long byteOffset = g.byteOffset(groupSelector.apply(i)); assertEquals(offsets[i], byteOffset); test/jdk/java/foreign/TestLayoutPaths.java line 236: > 234: for (int i = 0 ; i < 4 ; i++) { > 235: long bitOffset = g.byteOffset(sequenceElement(i)); > 236: assertEquals(offsets[i], bitOffset); Suggestion: long byteOffset = g.byteOffset(sequenceElement(i)); assertEquals(offsets[i], byteOffset); test/jdk/java/foreign/TestLayoutPaths.java line 246: > 244: byteOffsetHandle = byteOffsetHandle.asSpreader(long[].class, > indexes.length); > 245: long actualBitOffset = (long) > byteOffsetHandle.invokeExact(indexes); > 246: assertEquals(actualBitOffset, expectedByteOffset); Suggestion: long actualByteOffset = (long) byteOffsetHandle.invokeExact(indexes); assertEquals(actualByteOffset, expectedByteOffset); test/jdk/java/foreign/TestLayouts.java line 286: > 284: > 285: @Test(dataProvider="layoutsAndAlignments") > 286: public void testBadBitAlignment(MemoryLayout layout, long byteAlign) > { Suggestion: public void testBadByteAlignment(MemoryLayout layout, long byteAlign) { ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197051902 PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197057928 PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197053576 PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197054978 PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197055501 PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197056040 PR Review Comment: https://git.openjdk.org/jdk/pull/14013#discussion_r1197056551
