Skip to content

Commit

Permalink
[vm/ffi] Rewrite .ref calls in CFE
Browse files Browse the repository at this point in the history
This rewrites `Pointer<Struct>.ref` and `Pointer<Struct>[]` calls in
the CFE to skip the runtime entry when the type argument is constant.

The runtime entry is still used when the type argument is generic.
Forcing the type argument to be constant and removing the runtime entry
will be done in follow up CLs.

Bug: #38721
Bug: #44621

Removing the runtime entry speeds up `.ref` significantly.
before: FfiStruct.FieldLoadStore(RunTime): 18868.140186915887 us.
after: FfiStruct.FieldLoadStore(RunTime): 270.5877976190476 us.
Measurements from Linux x64 in JIT mode.

Closes: #38648

TEST=tests/ffi/structs_test.dart

Change-Id: I82abd930b5a9c5c78a8999c2bc49802d67d37534
Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,analyzer-nnbd-linux-release-try,app-kernel-linux-debug-x64-try,dart-sdk-linux-try,front-end-nnbd-linux-release-x64-try,pkg-linux-debug-try,vm-kernel-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/182265
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
  • Loading branch information
dcharkes authored and commit-bot@chromium.org committed Feb 2, 2021
1 parent 7cb49e2 commit 75c1748
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Coordinate extends ffi::Struct {
: super ffi::Struct::_fromPointer(#pointer)
;
static factory allocate(ffi::Allocator* allocator, core::double* x, core::double* y, ffi::Pointer<self::Coordinate*>* next) → self::Coordinate* {
return let final self::Coordinate* #t1 = ffi::StructPointer|get#ref<self::Coordinate*>(ffi::AllocatorAlloc|call<self::Coordinate*>(allocator)) in block {
return let final self::Coordinate* #t1 = new self::Coordinate::#fromTypedDataBase(allocator.{ffi::Allocator::allocate}<self::Coordinate*>(self::Coordinate::#sizeOf)!) in block {
#t1.{self::Coordinate::x} = x;
#t1.{self::Coordinate::y} = y;
#t1.{self::Coordinate::next} = next;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Coordinate extends ffi::Struct {
set next(ffi::Pointer<self::Coordinate> #externalFieldValue) → void
return ffi::_storeIntPtr(this.{ffi::Struct::_addressOf}, (#C18).{core::List::[]}(ffi::_abi()), #externalFieldValue.{ffi::Pointer::address});
static factory allocate(ffi::Allocator allocator, core::double x, core::double y, ffi::Pointer<self::Coordinate> next) → self::Coordinate {
return let final self::Coordinate #t1 = ffi::StructPointer|get#ref<self::Coordinate>(ffi::AllocatorAlloc|call<self::Coordinate>(allocator)) in block {
return let final self::Coordinate #t1 = new self::Coordinate::#fromTypedDataBase(allocator.{ffi::Allocator::allocate}<self::Coordinate>(self::Coordinate::#sizeOf)!) in block {
#t1.{self::Coordinate::x} = x;
#t1.{self::Coordinate::y} = y;
#t1.{self::Coordinate::next} = next;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Coordinate extends ffi::Struct {
set next(ffi::Pointer<self::Coordinate> #externalFieldValue) → void
return ffi::_storeIntPtr(this.{ffi::Struct::_addressOf}, (#C18).{core::List::[]}(ffi::_abi()), #externalFieldValue.{ffi::Pointer::address});
static factory allocate(ffi::Allocator allocator, core::double x, core::double y, ffi::Pointer<self::Coordinate> next) → self::Coordinate {
return let final self::Coordinate #t1 = ffi::StructPointer|get#ref<self::Coordinate>(ffi::AllocatorAlloc|call<self::Coordinate>(allocator)) in block {
return let final self::Coordinate #t1 = new self::Coordinate::#fromTypedDataBase(allocator.{ffi::Allocator::allocate}<self::Coordinate>(self::Coordinate::#sizeOf)!) in block {
#t1.{self::Coordinate::x} = x;
#t1.{self::Coordinate::y} = y;
#t1.{self::Coordinate::next} = next;
Expand Down
33 changes: 30 additions & 3 deletions pkg/vm/lib/transformations/ffi_use_sites.dart
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,13 @@ class _FfiUseSiteTransformer extends FfiTransformer {

_warningNativeTypeValid(nativeType, node, allowStructItself: false);

// TODO(http://dartbug.com/38721): Replace calls with direct
// constructor invocations.
// TODO(http://dartbug.com/38721): Change this to an error.
if (nativeType is TypeParameterType) {
// Do not rewire generic invocations.
return node;
}

return _replaceRef(node);
} else if (target == sizeOfMethod) {
final DartType nativeType = node.arguments.types[0];

Expand Down Expand Up @@ -439,6 +444,28 @@ class _FfiUseSiteTransformer extends FfiTransformer {
return StaticGet(field);
}

Expression _replaceRef(StaticInvocation node) {
final dartType = node.arguments.types[0];
final clazz = (dartType as InterfaceType).classNode;
final constructor = clazz.constructors
.firstWhere((c) => c.name == Name("#fromTypedDataBase"));
Expression pointer = NullCheck(node.arguments.positional[0]);
if (node.arguments.positional.length == 2) {
pointer = MethodInvocation(
pointer,
offsetByMethod.name,
Arguments([
MethodInvocation(
node.arguments.positional[1],
numMultiplication.name,
Arguments([_inlineSizeOf(dartType)]),
numMultiplication)
]),
offsetByMethod);
}
return ConstructorInvocation(constructor, Arguments([pointer]));
}

@override
visitMethodInvocation(MethodInvocation node) {
super.visitMethodInvocation(node);
Expand Down Expand Up @@ -470,7 +497,7 @@ class _FfiUseSiteTransformer extends FfiTransformer {
numMultiplication.name,
Arguments([inlineSizeOf]),
numMultiplication)
], types: node.arguments.types),
]),
offsetByMethod);
}
}
Expand Down
33 changes: 31 additions & 2 deletions tests/ffi/structs_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ void main() {
for (int i = 0; i < 100; i++) {
testStructAllocate();
testStructFromAddress();
testStructIndexedAccess();
testStructWithNulls();
testTypeTest();
testUtf8();
testDotDotRef();
}
}

/// allocates each coordinate separately in c memory
/// Allocates each coordinate separately in c memory.
void testStructAllocate() {
final c1 = calloc<Coordinate>()
..ref.x = 10.0
Expand Down Expand Up @@ -54,7 +55,7 @@ void testStructAllocate() {
calloc.free(c3);
}

/// allocates coordinates consecutively in c memory
/// Allocates coordinates consecutively in c memory.
void testStructFromAddress() {
Pointer<Coordinate> c1 = calloc(3);
Pointer<Coordinate> c2 = c1.elementAt(1);
Expand Down Expand Up @@ -84,6 +85,34 @@ void testStructFromAddress() {
calloc.free(c1);
}

/// Allocates coordinates consecutively in c memory.
void testStructIndexedAccess() {
Pointer<Coordinate> cs = calloc(3);
cs[0]
..x = 10.0
..y = 10.0
..next = cs.elementAt(2);
cs[1]
..x = 20.0
..y = 20.0
..next = cs;
cs[2]
..x = 30.0
..y = 30.0
..next = cs.elementAt(1);

Coordinate currentCoordinate = cs.ref;
Expect.equals(10.0, currentCoordinate.x);
currentCoordinate = currentCoordinate.next.ref;
Expect.equals(30.0, currentCoordinate.x);
currentCoordinate = currentCoordinate.next.ref;
Expect.equals(20.0, currentCoordinate.x);
currentCoordinate = currentCoordinate.next.ref;
Expect.equals(10.0, currentCoordinate.x);

calloc.free(cs);
}

void testStructWithNulls() {
final coordinate = calloc<Coordinate>()
..ref.x = 10.0
Expand Down
33 changes: 31 additions & 2 deletions tests/ffi_2/structs_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ void main() {
for (int i = 0; i < 100; i++) {
testStructAllocate();
testStructFromAddress();
testStructIndexedAccess();
testStructWithNulls();
testTypeTest();
testUtf8();
testDotDotRef();
}
}

/// allocates each coordinate separately in c memory
/// Allocates each coordinate separately in c memory.
void testStructAllocate() {
final c1 = calloc<Coordinate>()
..ref.x = 10.0
Expand Down Expand Up @@ -54,7 +55,7 @@ void testStructAllocate() {
calloc.free(c3);
}

/// allocates coordinates consecutively in c memory
/// Allocates coordinates consecutively in c memory.
void testStructFromAddress() {
Pointer<Coordinate> c1 = calloc(3);
Pointer<Coordinate> c2 = c1.elementAt(1);
Expand Down Expand Up @@ -84,6 +85,34 @@ void testStructFromAddress() {
calloc.free(c1);
}

/// Allocates coordinates consecutively in c memory.
void testStructIndexedAccess() {
Pointer<Coordinate> cs = calloc(3);
cs[0]
..x = 10.0
..y = 10.0
..next = cs.elementAt(2);
cs[1]
..x = 20.0
..y = 20.0
..next = cs;
cs[2]
..x = 30.0
..y = 30.0
..next = cs.elementAt(1);

Coordinate currentCoordinate = cs.ref;
Expect.equals(10.0, currentCoordinate.x);
currentCoordinate = currentCoordinate.next.ref;
Expect.equals(30.0, currentCoordinate.x);
currentCoordinate = currentCoordinate.next.ref;
Expect.equals(20.0, currentCoordinate.x);
currentCoordinate = currentCoordinate.next.ref;
Expect.equals(10.0, currentCoordinate.x);

calloc.free(cs);
}

void testStructWithNulls() {
final coordinate = calloc<Coordinate>()
..ref.x = 10.0
Expand Down

0 comments on commit 75c1748

Please sign in to comment.