RFR: 8308276: Change layout API to work with bytes, not bits [v4]
Per Minborg
pminborg at openjdk.org
Mon May 22 14:32:01 UTC 2023
On Mon, 22 May 2023 10:27:04 GMT, Maurizio Cimadamore <mcimadamore at 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
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
>
> - Merge branch 'master' into bits_to_bytes_layouts
> - Address review comments
> - Update test/jdk/java/foreign/TestLayouts.java
>
> Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
> - Update test/jdk/java/foreign/TestLayoutPaths.java
>
> Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
> - Update test/jdk/java/foreign/TestLayoutPaths.java
>
> Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
> - Update test/jdk/java/foreign/TestLayoutPaths.java
>
> Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
> - Update src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java
>
> Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
> - Drop spurious reference to "bit"/"bits" in both javadoc and impl
> - Fix tests
> - Fix bad assertion in AbstractValueLayouts
> Fix vector tests to use withByteAlignment
> - ... and 1 more: https://git.openjdk.org/jdk/compare/8aa50288...145fb32d
LGTM.
-------------
Marked as reviewed by pminborg (Committer).
PR Review: https://git.openjdk.org/jdk/pull/14013#pullrequestreview-1436768257
More information about the core-libs-dev
mailing list