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.

Sign up for GitHub

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

Jump to bottom

[vm/ffi] API: Pointer load and store as extension methods #37773

Closed
sjindel-google opened this issue Aug 7, 2019 · 9 comments
Closed

[vm/ffi] API: Pointer load and store as extension methods #37773

sjindel-google opened this issue Aug 7, 2019 · 9 comments
Assignees
@dcharkes
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi part-of:pointer-api
Milestone
D26 Release

Comments

@sjindel-google
Copy link
Contributor

sjindel-google commented Aug 7, 2019 •
edited by dcharkes
Loading

These methods would be better exposed as extension methods:

update from @dcharkes: I split this issue into three to keep the CL sizes manageable.
@sjindel-google sjindel-google added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi labels Aug 7, 2019
@sjindel-google
Copy link
Contributor Author

/cc @johnniwinther

@sjindel-google
Copy link
Contributor Author

Putting this in Q3 stable since extension methods should be usable by then.

@dcharkes dcharkes mentioned this issue Sep 2, 2019
Closed
@dcharkes dcharkes added this to Flutter MVP in Dart VM FFI via automation Sep 3, 2019
@dcharkes dcharkes mentioned this issue Sep 26, 2019
Closed
@dcharkes
Copy link
Contributor

dcharkes commented Sep 27, 2019 •
edited
Loading

  • Pointer.asExternalTypedData (but keep original)

@sjindel-google, how did you envision this? Instance methods shadow extension methods.

For reference for load and store: the design discussion: #37284.

This was referenced Sep 27, 2019
Closed
Closed
@dcharkes dcharkes changed the title [vm/ffi] Move Pointer and Struct operations to extension methods [vm/ffi] API: Pointer load and store as extension methods Sep 27, 2019
@dcharkes dcharkes self-assigned this Sep 27, 2019
@dcharkes
Copy link
Contributor

I've made CLs for the API change and updating the tests, samples, and benchmarks. Feel free to leave comments on those.

Note, we cannot land these CLs as long as the extension methods are behind a flag as experiment.

@dcharkes
Copy link
Contributor

Do we want to keep load and store as @deprecated for a while? Or would we like to remove them right away? If we deprecate them, how long should we keep them? Till Dart 2.7?

Pro for deleting them: We do not have deprecated stuff in the ffi 1.0 release.
Pro for making them deprecated for a while: We don't break existing customers right away.

Maybe we should delete them right away. We have been upfront about API changes, and it will clean up the API and our internal implementation.

What is your opinion @sjindel-google @mkustermann?

@sjindel-google
Copy link
Contributor Author

I vote for breaking them, but we should land all our breaking changes for 1.0 at once.

@dcharkes
Copy link
Contributor

dcharkes commented Sep 27, 2019 •
edited
Loading

we should land all our breaking changes for 1.0 at once.

👍 Yeah, or we would break people on master and g3 multiple times.

Edit: we can land the extension method additions early, but removing the old ones we should do all together.

@dcharkes dcharkes mentioned this issue Sep 30, 2019
Closed
@dcharkes dcharkes added this to the D26 Release milestone Sep 30, 2019
@dcharkes dcharkes mentioned this issue Sep 30, 2019
Closed
@dcharkes
Copy link
Contributor

Note, we cannot land these CLs as long as the extension methods are behind a flag as experiment.

@johnniwinther has just turned on the flag by default. We are unblocked. 👍

@mkustermann mkustermann mentioned this issue Oct 6, 2019
Closed
@dcharkes
Copy link
Contributor

dcharkes commented Oct 7, 2019

/cc @lrhn for his view on .val and .ref.

dart-bot pushed a commit that referenced this issue Oct 8, 2019
@dcharkes
[vm/ffi] Pointer load and store as extension methods
597cd06 
Issue: #37773
Change-Id: I836d6305b613cf05590d872874f4517831be3e08
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-dartkb-linux-debug-simarm64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-dartkb-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-mac-debug-simdbc64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-reload-mac-release-simdbc64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-precomp-mac-release-simarm_x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/118992
Reviewed-by: Samir Jindel <sjindel@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2019
@dcharkes
[vm/ffi] Migrate tests/samples/benchmarks to extension methods
ee0b1df 
I used the regex replaces documented on the `load` and `store` deprecated methods.
I manually replaced some `.val` to `.ref` when a regex could not detect whether something was a primitive value or a struct.

Issue: #37773

Change-Id: I3534b6dd00d9ac45fa1a11fe75c80fb3cccc07dc
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-dartkb-linux-debug-simarm64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-dartkb-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-mac-debug-simdbc64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-reload-mac-release-simdbc64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-precomp-mac-release-simarm_x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/118993
Reviewed-by: Samir Jindel <sjindel@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
@dcharkes dcharkes closed this as completed Oct 8, 2019
Dart VM FFI automation moved this from Flutter MVP to Done Oct 8, 2019
@dcharkes dcharkes mentioned this issue Oct 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dcharkes dcharkes

Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi part-of:pointer-api
Projects
None yet
Milestone
D26 Release
Development

No branches or pull requests
2 participants
@dcharkes @sjindel-google