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

[vm/ffi] Optimize Pointer<Struct> operations #38648

Closed
dcharkes opened this issue Sep 30, 2019 · 3 comments
Closed

[vm/ffi] Optimize Pointer<Struct> operations #38648

dcharkes opened this issue Sep 30, 2019 · 3 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Sep 30, 2019

Hot loops with structs usually contain loading a struct in the hot loop. For example

void doStoreInt32(Pointer<VeryLargeStruct> pointer, int length) {
  for (int i = 0; i < length; i++) {
    pointer.elementAt(i).load<VeryLargeStruct>().c = 1;
    // pointer[i].c = 1; // With extension method syntax.
  }
}

This is currently slow because (1) loading a struct from a pointer is a runtime entry (2) looking up the size of a struct is a runtime entry.

1. Loading a struct from a Pointer

The way that we implement this runtime entry is:

// Find <class name>.#fromPointer constructor and call it.

https://github.com/dart-lang/sdk/tree/master/runtime/lib/ffi.cc

The constructor is generated in kernel

// Add a constructor which 'load' can use.
// C.#fromPointer(Pointer<Void> address) : super.fromPointer(address);

https://github.com/dart-lang/sdk/tree/master/pkg/vm/lib/transformations/ffi_definitions.dart

It would be simple to generate IL per call site to call the right constructor, but our recognized methods generate method bodies, not specialized per call site. We could use a hashtable from class to method to make a generic method body, but we would need to populate that hashtable at some point.

2. Looking up the size of a struct

Same kind of problem: we find the right #sizeOf field, and read its value in the runtime entry.

This would also be trivial per (non-generic) call site.

@sjindel-google @mkustermann Any suggestion on how to implement this optimization?

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Sep 30, 2019
@dcharkes dcharkes self-assigned this Sep 30, 2019
@sjindel-google
Copy link
Contributor

We should just require that the struct type being loaded is statically known, as in C.

@dcharkes dcharkes changed the title [vm/ffi] Optimize Pointer<Struct> load [vm/ffi] Optimize Pointer<Struct> operations Oct 2, 2019
@dcharkes
Copy link
Contributor Author

dcharkes commented Oct 2, 2019

As discussed with @mkustermann offline, we can optimize the load and sizeOf of statically known type arguments by rewiring in the frontend.

We need to keep supporting generic invocations, at least for sizeOf, if we want support package:ffi exposing allocate with the current signature (#38520):

malloc = // lookedUpSymbol

Pointer<T> allocate<T>(int count){
  final int size = sizeOf<T>() * count;
  final int addr = malloc(size);
  return Pointer.fromAddress(addr);
}

@dcharkes
Copy link
Contributor Author

Pointer.ref is dominating benchmarks with structs: https://github.com/mannprerak2/exp-ffi-dart/tree/master/json_parser_ffi#observatory-cpu-profile

Thanks for the benchmark @mannprerak2!

dart-bot pushed a commit that referenced this issue Jan 5, 2021
Now that we have nested structs, objects a subtype of `Struct` can be
backed by either a `Pointer` or a `TypedData`. Having this accessor is
misleading.

Instead of passing a struct around (which could be backed by either),
the `Pointer<T extends Struct>` should be passed around and `.ref`
should be used everywhere when access to the backing pointer is
required.

Issue: #40667

Related issues:
* Optimize .ref #38648
* Support tree shaking of structs #38721

Change-Id: I3c73423480b91c00639df886bf1d6ac2e444beab
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/177581
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
@dcharkes dcharkes added this to the February Beta Release milestone Jan 22, 2021
dart-bot pushed a commit that referenced this issue Feb 4, 2021
After https://dart-review.googlesource.com/c/sdk/+/180190 the runtime
entry has become dead code.

This CL keeps the runtime entry itself but makes it unreachable as was
the suggestion on previous a CL removing runtime entries:
https://dart-review.googlesource.com/c/sdk/+/169406

Bug: #38648
Bug: #38721

TEST=tests/ffi/vmspecific_static_checks_test.dart
TEST=tests/ffi/*struct*test_.dart

Change-Id: I84c5c925215b9dbd999826fb390df91d8050e1dd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/182627
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

2 participants