Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MemoryLayout::byteOffset #157

Closed

Conversation

@ChrisHegarty
Copy link
Member

@ChrisHegarty ChrisHegarty commented May 11, 2020

Hi,

As part of feedback on the Foreign Memory API (when experimenting with
its usage internally in the JDK), a small number of potential usability
enhancements could be made to the API. This is the first such.

This change:

  1. renames MemoryLayout::offset to MemoryLayout::bitOffset, and
  2. adds MemoryLayout::byteOffset

This allows for easier interoperation with MemoryAddress, which deals
with offsets and lengths in terms of bytes (rather than bits), e.g.
addr.addOffset(layout.byteOffset(groupElement("foo")))

This brings a nice symmetry to the API; there is a trio of bit-wise
methods: bitAlignment, bitOffset, bitSize, and their matching byte-wise
counterparts; byteAlignment, byteOffset, byteSize.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/157/head:pull/157
$ git checkout pull/157

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 11, 2020

👋 Welcome back chegar! A progress list of the required criteria for merging this PR into foreign-jextract will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 11, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 11, 2020

Webrevs

@JornVernee
Copy link
Member

@JornVernee JornVernee commented May 11, 2020

This seems like it should go to the foreign-memaccess branch, but is currently targeting the foreign-jextract branch. If you click the "Edit" button on the right next to the title of the PR, you should be able to change the target branch from the drop-down just below the PR title.

Copy link
Member

@JornVernee JornVernee left a comment

Looks good, thanks for the thorough testing!

@openjdk
Copy link

@openjdk openjdk bot commented May 11, 2020

@ChrisHegarty This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

Add MemoryLayout::byteOffset

Reviewed-by: jvernee, mcimadamore
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the foreign-memaccess branch:

  • d3b7d98: Alternative scalable MemoryScope (followup)

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-memaccess into your branch, and then specify the current head hash when integrating, like this: /integrate d3b7d9817a86cd2ee26973f9fe465a3b011bc31e.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JornVernee, @mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 11, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 11, 2020

Mailing list message from Ty Young on panama-dev:

On 5/11/20 3:36 AM, Chris Hegarty wrote:

Hi,

As part of feedback on the Foreign Memory API (when experimenting with
its usage internally in the JDK), a small number of potential usability
enhancements could be made to the API. This is the first such.

This change:
1) renames MemoryLayout::offset to MemoryLayout::bitOffset, and
2) adds MemoryLayout::byteOffset

This allows for easier interoperation with MemoryAddress, which deals
with offsets and lengths in terms of bytes (rather than bits), e.g.
addr.addOffset(layout.byteOffset(groupElement("foo")))

This brings a nice symmetry to the API; there is a trio of bit-wise
methods: bitAlignment, bitOffset, bitSize, and their matching byte-wise
counterparts; byteAlignment, byteOffset, byteSize.

This reminds me something I meant to bring up awhile ago: it would be
nice to have a counterpart method "ofPaddingBytes" and pre-defined
layouts like "PAD_BYTE_1" to the existing Bit based versions.

Could those be added too?

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

Commit messages:
- fix typo in javadoc example
- Initial MemoryLayout::byteOffset and test changes

Changes: https://git.openjdk.java.net/panama-foreign/pull/157/files
Webrev: https://webrevs.openjdk.java.net/panama-foreign/157/webrev.00
Stats: 131 lines in 2 files changed: 103 ins; 1 del; 27 mod
Patch: https://git.openjdk.java.net/panama-foreign/pull/157.diff
Fetch: git fetch https://git.openjdk.java.net/panama-foreign pull/157/head:pull/157

PR: https://git.openjdk.java.net/panama-foreign/pull/157

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks very good - many thanks!

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 11, 2020

This reminds me something I meant to bring up awhile ago: it would be
nice to have a counterpart method "ofPaddingBytes" and pre-defined
layouts like "PAD_BYTE_1" to the existing Bit based versions.

Right now all factories take bits (not bytes) as input, as that's more "primitive" and scales more to things like packed layouts. That said, I see your point. My feeling is that layout construction typically happens once, and then information is derived from the layout. When you construct you don't care much whether to use bits or bytes (other than for stylistic preferences) - but when you access layout information, you might be interested in getting either bits or bytes out, depending on which code you might need to interact with. So, in my view, fixing methods like "offset" takes the priority here. We'll keep evaluating against use cases and see how annoying that is. Thanks for bringing this up.

@ChrisHegarty ChrisHegarty changed the base branch from foreign-jextract to foreign-memaccess May 11, 2020
@ChrisHegarty
Copy link
Member Author

@ChrisHegarty ChrisHegarty commented May 11, 2020

This seems like it should go to the foreign-memaccess branch, but is currently targeting the foreign-jextract branch. If you click the "Edit" button on the right next to the title of the PR, you should be able to change the target branch from the drop-down just below the PR title.

Thanks Jorn, the intent was to target the foreign-memaccess branch. PR target branch updated.

@ChrisHegarty
Copy link
Member Author

@ChrisHegarty ChrisHegarty commented May 11, 2020

/integrate

@openjdk openjdk bot added the sponsor label May 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 11, 2020

@ChrisHegarty
Your change (at version 47b2be6) is now ready to be sponsored by a Committer.

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 11, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented May 11, 2020

@mcimadamore @ChrisHegarty The following commits have been pushed to foreign-memaccess since your change was applied:

  • d3b7d98: Alternative scalable MemoryScope (followup)

Your commit was automatically rebased without conflicts.

Pushed as commit 1bd6563.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants