Re: RFR: 8308276: Change layout API to work with bytes, not bits [v4]
On Mon, 22 May 2023 10:27:04 GMT, Maurizio Cimadamore 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 > - Update test/jdk/java/foreign/TestLayoutPaths.java > >Co-authored-by: Paul Sandoz > - Update test/jdk/java/foreign/TestLayoutPaths.java > >Co-authored-by: Paul Sandoz > - Update test/jdk/java/foreign/TestLayoutPaths.java > >Co-authored-by: Paul Sandoz > - Update > src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java > >Co-authored-by: Paul Sandoz > - 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
Re: RFR: 8308276: Change layout API to work with bytes, not bits [v4]
> 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 - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java Co-authored-by: Paul Sandoz - 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 - Changes: https://git.openjdk.org/jdk/pull/14013/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14013&range=03 Stats: 724 lines in 93 files changed: 3 ins; 197 del; 524 mod Patch: https://git.openjdk.org/jdk/pull/14013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14013/head:pull/14013 PR: https://git.openjdk.org/jdk/pull/14013
