Skip to content

Commit 9f33e8d

Browse files
dcharkesdcharkes
committed
[vm/ffi] Pointer optimize indexed load and store
Follow up of https://dart-review.googlesource.com/c/sdk/+/117547 This gets rid of unnecessary allocations in hot loops with indexed loads and stores. Issue: #38172 Change-Id: I37a4b1aba00084e465d47cce79bb9963e1afc104 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/+/119645 Reviewed-by: Martin Kustermann <[email protected]>
1 parent e1c159d commit 9f33e8d

File tree

12 files changed

+403
-304
lines changed

12 files changed

+403
-304
lines changed

‎DEPS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ vars = {
8484
# For more details, see https://github.com/dart-lang/sdk/issues/30164
8585
"dart_style_tag": "1.3.1", # Please see the note above before updating.
8686

87+
"args_tag" : "1.5.2",
8788
"dartdoc_tag" : "v0.28.7",
8889
"ffi_tag": "f46c1f42c6f7f1938f2ff27c573da72d57c47ded",
8990
"fixnum_tag": "0.10.9",
@@ -276,6 +277,8 @@ deps = {
276277
Var("dart_git") + "dart_style.git" + "@" + Var("dart_style_tag"),
277278
Var("dart_root") + "/third_party/pkg/dart2js_info":
278279
Var("dart_git") + "dart2js_info.git" + "@" + Var("dart2js_info_tag"),
280+
Var("dart_root") + "/third_party/pkg/args":
281+
Var("dart_git") + "args.git" + "@" + Var("args_tag"),
279282
Var("dart_root") + "/third_party/pkg/dartdoc":
280283
Var("dart_git") + "dartdoc.git" + "@" + Var("dartdoc_tag"),
281284
Var("dart_root") + "/third_party/pkg/ffi":

‎pkg/vm/lib/transformations/ffi_use_sites.dart

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,8 @@ class _FfiUseSiteTransformer extends FfiTransformer {
336336
];
337337
return StaticInvocation(
338338
optimizedTypes.contains(nt) ? loadMethods[nt] : loadStructMethod,
339-
Arguments([node.receiver], types: typeArguments));
339+
Arguments([node.receiver, ConstantExpression(IntConstant(0))],
340+
types: typeArguments));
340341
} else if (target == storeMethod) {
341342
final Expression storeValue = node.arguments.positional.single;
342343
final DartType dartType = storeValue.getStaticType(env);
@@ -357,8 +358,11 @@ class _FfiUseSiteTransformer extends FfiTransformer {
357358
final typeArguments = [
358359
if (nt == NativeType.kPointer) _pointerTypeGetTypeArg(nativeType)
359360
];
360-
return StaticInvocation(storeMethods[nt],
361-
Arguments([node.receiver, storeValue], types: typeArguments));
361+
return StaticInvocation(
362+
storeMethods[nt],
363+
Arguments(
364+
[node.receiver, ConstantExpression(IntConstant(0)), storeValue],
365+
types: typeArguments));
362366
} else if (target == elementAtMethod) {
363367
// TODO(37773): When moving to extension methods we can get rid of
364368
// this rewiring.

‎runtime/lib/ffi.cc

Lines changed: 44 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,12 @@ DEFINE_NATIVE_ENTRY(Ffi_address, 0, 1) {
129129

130130
static RawObject* LoadValueNumeric(Zone* zone,
131131
const Pointer& target,
132-
classid_t type_cid) {
133-
const size_t address = target.NativeAddress();
132+
classid_t type_cid,
133+
const Integer& index) {
134+
// TODO(36370): Make representation consistent with kUnboxedFfiIntPtr.
135+
const size_t address =
136+
target.NativeAddress() + static_cast<intptr_t>(index.AsInt64Value()) *
137+
compiler::ffi::ElementSizeInBytes(type_cid);
134138
switch (type_cid) {
135139
case kFfiInt8Cid:
136140
return Integer::New(*reinterpret_cast<int8_t*>(address));
@@ -160,23 +164,28 @@ static RawObject* LoadValueNumeric(Zone* zone,
160164
}
161165

162166
#define DEFINE_NATIVE_ENTRY_LOAD(type) \
163-
DEFINE_NATIVE_ENTRY(Ffi_load##type, 0, 1) { \
167+
DEFINE_NATIVE_ENTRY(Ffi_load##type, 0, 2) { \
164168
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, pointer, arguments->NativeArgAt(0)); \
165-
return LoadValueNumeric(zone, pointer, kFfi##type##Cid); \
169+
GET_NON_NULL_NATIVE_ARGUMENT(Integer, index, arguments->NativeArgAt(1)); \
170+
return LoadValueNumeric(zone, pointer, kFfi##type##Cid, index); \
166171
}
167172
CLASS_LIST_FFI_NUMERIC(DEFINE_NATIVE_ENTRY_LOAD)
168173
#undef DEFINE_NATIVE_ENTRY_LOAD
169174

170-
DEFINE_NATIVE_ENTRY(Ffi_loadPointer, 1, 1) {
175+
DEFINE_NATIVE_ENTRY(Ffi_loadPointer, 1, 2) {
171176
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, pointer, arguments->NativeArgAt(0));
177+
GET_NON_NULL_NATIVE_ARGUMENT(Integer, index, arguments->NativeArgAt(1));
178+
172179
const auto& pointer_type_arg =
173180
AbstractType::Handle(zone, pointer.type_argument());
174-
175-
const auto& type_arg =
181+
const AbstractType& type_arg =
176182
AbstractType::Handle(TypeArguments::Handle(pointer_type_arg.arguments())
177183
.TypeAt(Pointer::kNativeTypeArgPos));
178184

179-
const size_t address = pointer.NativeAddress();
185+
// TODO(36370): Make representation consistent with kUnboxedFfiIntPtr.
186+
const size_t address =
187+
pointer.NativeAddress() +
188+
static_cast<intptr_t>(index.AsInt64Value()) * SizeOf(pointer_type_arg);
180189

181190
return Pointer::New(type_arg, *reinterpret_cast<uword*>(address));
182191
}
@@ -208,19 +217,31 @@ static RawObject* LoadValueStruct(Zone* zone,
208217
return new_object.raw();
209218
}
210219

211-
DEFINE_NATIVE_ENTRY(Ffi_loadStruct, 0, 1) {
220+
DEFINE_NATIVE_ENTRY(Ffi_loadStruct, 0, 2) {
212221
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, pointer, arguments->NativeArgAt(0));
213222
const AbstractType& pointer_type_arg =
214223
AbstractType::Handle(pointer.type_argument());
224+
GET_NON_NULL_NATIVE_ARGUMENT(Integer, index, arguments->NativeArgAt(1));
225+
226+
// TODO(36370): Make representation consistent with kUnboxedFfiIntPtr.
227+
const size_t address =
228+
pointer.NativeAddress() +
229+
static_cast<intptr_t>(index.AsInt64Value()) * SizeOf(pointer_type_arg);
230+
const Pointer& pointer_offset =
231+
Pointer::Handle(zone, Pointer::New(pointer_type_arg, address));
215232

216-
return LoadValueStruct(zone, pointer, pointer_type_arg);
233+
return LoadValueStruct(zone, pointer_offset, pointer_type_arg);
217234
}
218235

219236
static void StoreValueNumeric(Zone* zone,
220237
const Pointer& pointer,
221238
classid_t type_cid,
239+
const Integer& index,
222240
const Instance& new_value) {
223-
uint8_t* const address = reinterpret_cast<uint8_t*>(pointer.NativeAddress());
241+
// TODO(36370): Make representation consistent with kUnboxedFfiIntPtr.
242+
const size_t address =
243+
pointer.NativeAddress() + static_cast<intptr_t>(index.AsInt64Value()) *
244+
compiler::ffi::ElementSizeInBytes(type_cid);
224245
switch (type_cid) {
225246
case kFfiInt8Cid:
226247
*reinterpret_cast<int8_t*>(address) = AsInteger(new_value).AsInt64Value();
@@ -269,23 +290,20 @@ static void StoreValueNumeric(Zone* zone,
269290
}
270291

271292
#define DEFINE_NATIVE_ENTRY_STORE(type) \
272-
DEFINE_NATIVE_ENTRY(Ffi_store##type, 0, 2) { \
293+
DEFINE_NATIVE_ENTRY(Ffi_store##type, 0, 3) { \
273294
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, pointer, arguments->NativeArgAt(0)); \
274-
GET_NATIVE_ARGUMENT(Instance, new_value, arguments->NativeArgAt(1)); \
275-
if (new_value.IsNull()) { \
276-
const String& error = String::Handle( \
277-
String::NewFormatted("Argument to Pointer.store is null.")); \
278-
Exceptions::ThrowArgumentError(error); \
279-
} \
280-
StoreValueNumeric(zone, pointer, kFfi##type##Cid, new_value); \
295+
GET_NON_NULL_NATIVE_ARGUMENT(Integer, index, arguments->NativeArgAt(1)); \
296+
GET_NON_NULL_NATIVE_ARGUMENT(Instance, value, arguments->NativeArgAt(2)); \
297+
StoreValueNumeric(zone, pointer, kFfi##type##Cid, index, value); \
281298
return Object::null(); \
282299
}
283300
CLASS_LIST_FFI_NUMERIC(DEFINE_NATIVE_ENTRY_STORE)
284301
#undef DEFINE_NATIVE_ENTRY_STORE
285302

286-
DEFINE_NATIVE_ENTRY(Ffi_storePointer, 0, 2) {
303+
DEFINE_NATIVE_ENTRY(Ffi_storePointer, 0, 3) {
287304
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, pointer, arguments->NativeArgAt(0));
288-
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, new_value, arguments->NativeArgAt(1));
305+
GET_NON_NULL_NATIVE_ARGUMENT(Integer, index, arguments->NativeArgAt(1));
306+
GET_NON_NULL_NATIVE_ARGUMENT(Pointer, new_value, arguments->NativeArgAt(2));
289307
AbstractType& pointer_type_arg =
290308
AbstractType::Handle(pointer.type_argument());
291309

@@ -300,8 +318,11 @@ DEFINE_NATIVE_ENTRY(Ffi_storePointer, 0, 2) {
300318
}
301319

302320
ASSERT(IsPointerType(pointer_type_arg));
303-
uword* slot = reinterpret_cast<uword*>(pointer.NativeAddress());
304-
*slot = new_value.NativeAddress();
321+
// TODO(36370): Make representation consistent with kUnboxedFfiIntPtr.
322+
const size_t address =
323+
pointer.NativeAddress() +
324+
static_cast<intptr_t>(index.AsInt64Value()) * SizeOf(pointer_type_arg);
325+
*reinterpret_cast<uword*>(address) = new_value.NativeAddress();
305326
return Object::null();
306327
}
307328

‎runtime/tools/ffi/sdk_lib_ffi_generator.dart

Lines changed: 27 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
import 'dart:io';
1111

12+
import 'package:args/args.dart';
13+
1214
//
1315
// Configuration.
1416
//
@@ -32,10 +34,11 @@ const Map<String, String> nativeToDartType = {
3234
//
3335

3436
main(List<String> arguments) {
35-
final parsedArgs = parseArguments(arguments);
37+
final args = argParser().parse(arguments);
38+
Uri path = Uri.parse(args['path']);
3639

37-
generate(parsedArgs.path, "ffi.g.dart", generatePublicExtension);
38-
generate(parsedArgs.path, "ffi_patch.g.dart", generatePatchExtension);
40+
generate(path, "ffi.g.dart", generatePublicExtension);
41+
generate(path, "ffi_patch.g.dart", generatePatchExtension);
3942
}
4043

4144
void generate(Uri path, String fileName,
@@ -48,7 +51,7 @@ void generate(Uri path, String fileName,
4851
generateFooter(buffer);
4952

5053
final fullPath = path.resolve(fileName).path;
51-
new File(fullPath).writeAsStringSync(buffer.toString());
54+
File(fullPath).writeAsStringSync(buffer.toString());
5255
final fmtResult = Process.runSync(dartfmtPath().path, ["-w", fullPath]);
5356
if (fmtResult.exitCode != 0) {
5457
throw Exception(
@@ -81,13 +84,13 @@ void generatePublicExtension(
8184
""";
8285

8386
final storeTruncate =
84-
isInt(nativeType) ? storeTrunctateInt : storeTrunctateDouble;
87+
_isInt(nativeType) ? storeTrunctateInt : storeTrunctateDouble;
8588

8689
final loadSignExtendInt = """
8790
/// Note that ints are signextended.
8891
""";
8992

90-
final loadSignExtend = isInt(nativeType) ? loadSignExtendInt : "";
93+
final loadSignExtend = _isInt(nativeType) ? loadSignExtendInt : "";
9194

9295
// TODO(dartdoc-bug): Use [] instead of ``, once issue
9396
// https://github.com/dart-lang/dartdoc/issues/2039 is fixed.
@@ -130,17 +133,17 @@ void generatePatchExtension(
130133
StringBuffer buffer, String nativeType, String dartType) {
131134
buffer.write("""
132135
extension ${nativeType}Pointer on Pointer<$nativeType> {
133-
@patch
134-
$dartType get value => _load$nativeType(this);
136+
@patch
137+
$dartType get value => _load$nativeType(this, 0);
135138
136139
@patch
137-
void set value($dartType value) => _store$nativeType(this, value);
140+
set value($dartType value) => _store$nativeType(this, 0, value);
138141
139142
@patch
140-
$dartType operator [](int index) => this.elementAt(index).value;
143+
$dartType operator [](int index) => _load$nativeType(this, index);
141144
142145
@patch
143-
void operator []=(int index, $dartType value) => this.elementAt(index).value = value;
146+
operator []=(int index, $dartType value) => _store$nativeType(this, index, value);
144147
}
145148
146149
""");
@@ -160,34 +163,21 @@ void generateFooter(StringBuffer buffer) {
160163
// Helper functions.
161164
//
162165

163-
bool isInt(String type) => type.startsWith("Int") || type.startsWith("Uint");
164-
165-
class Arguments {
166-
final Uri path;
167-
Arguments(this.path);
168-
}
169-
170-
Arguments parseArguments(List<String> arguments) {
171-
final parsedArgs = Map<String, dynamic>();
172-
String flag = null;
173-
for (final String arg in arguments) {
174-
if (flag == "path") {
175-
parsedArgs[flag] = Uri.parse(arg);
176-
flag = null;
177-
} else if (arg == "-p" || arg == "--path") {
178-
flag = "path";
179-
} else {
180-
throw Exception("Unknown argument: $arg");
181-
}
182-
}
166+
bool _isInt(String type) => type.startsWith("Int") || type.startsWith("Uint");
183167

184-
Uri path = parsedArgs["path"];
185-
if (path == null) {
186-
path = Platform.script;
187-
print("No path provided, generating files next to generator.");
188-
}
168+
final Uri _containingFolder = File.fromUri(Platform.script).parent.uri;
189169

190-
return Arguments(path);
170+
ArgParser argParser() {
171+
final parser = ArgParser(allowTrailingOptions: false);
172+
parser.addOption('path',
173+
abbr: 'p',
174+
help: 'Path to generate the files at.',
175+
defaultsTo: _containingFolder.toString());
176+
parser.addFlag('help', abbr: 'h', help: 'Display usage information.',
177+
callback: (help) {
178+
if (help) print(parser.usage);
179+
});
180+
return parser;
191181
}
192182

193183
Uri dartfmtPath() {

‎runtime/vm/bootstrap_natives.h

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -372,31 +372,31 @@ namespace dart {
372372
V(VMService_spawnUriNotify, 2) \
373373
V(Ffi_allocate, 1) \
374374
V(Ffi_free, 1) \
375-
V(Ffi_loadInt8, 1) \
376-
V(Ffi_loadInt16, 1) \
377-
V(Ffi_loadInt32, 1) \
378-
V(Ffi_loadInt64, 1) \
379-
V(Ffi_loadUint8, 1) \
380-
V(Ffi_loadUint16, 1) \
381-
V(Ffi_loadUint32, 1) \
382-
V(Ffi_loadUint64, 1) \
383-
V(Ffi_loadIntPtr, 1) \
384-
V(Ffi_loadFloat, 1) \
385-
V(Ffi_loadDouble, 1) \
386-
V(Ffi_loadPointer, 1) \
387-
V(Ffi_loadStruct, 1) \
388-
V(Ffi_storeInt8, 2) \
389-
V(Ffi_storeInt16, 2) \
390-
V(Ffi_storeInt32, 2) \
391-
V(Ffi_storeInt64, 2) \
392-
V(Ffi_storeUint8, 2) \
393-
V(Ffi_storeUint16, 2) \
394-
V(Ffi_storeUint32, 2) \
395-
V(Ffi_storeUint64, 2) \
396-
V(Ffi_storeIntPtr, 2) \
397-
V(Ffi_storeFloat, 2) \
398-
V(Ffi_storeDouble, 2) \
399-
V(Ffi_storePointer, 2) \
375+
V(Ffi_loadInt8, 2) \
376+
V(Ffi_loadInt16, 2) \
377+
V(Ffi_loadInt32, 2) \
378+
V(Ffi_loadInt64, 2) \
379+
V(Ffi_loadUint8, 2) \
380+
V(Ffi_loadUint16, 2) \
381+
V(Ffi_loadUint32, 2) \
382+
V(Ffi_loadUint64, 2) \
383+
V(Ffi_loadIntPtr, 2) \
384+
V(Ffi_loadFloat, 2) \
385+
V(Ffi_loadDouble, 2) \
386+
V(Ffi_loadPointer, 2) \
387+
V(Ffi_loadStruct, 2) \
388+
V(Ffi_storeInt8, 3) \
389+
V(Ffi_storeInt16, 3) \
390+
V(Ffi_storeInt32, 3) \
391+
V(Ffi_storeInt64, 3) \
392+
V(Ffi_storeUint8, 3) \
393+
V(Ffi_storeUint16, 3) \
394+
V(Ffi_storeUint32, 3) \
395+
V(Ffi_storeUint64, 3) \
396+
V(Ffi_storeIntPtr, 3) \
397+
V(Ffi_storeFloat, 3) \
398+
V(Ffi_storeDouble, 3) \
399+
V(Ffi_storePointer, 3) \
400400
V(Ffi_address, 1) \
401401
V(Ffi_fromAddress, 1) \
402402
V(Ffi_sizeOf, 0) \

‎runtime/vm/compiler/frontend/base_flow_graph_builder.cc

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,34 @@ Fragment BaseFlowGraphBuilder::SmiBinaryOp(Token::Kind kind,
762762
return Fragment(instr);
763763
}
764764

765+
Fragment BaseFlowGraphBuilder::BinaryIntegerOp(Token::Kind kind,
766+
Representation representation,
767+
bool is_truncating) {
768+
ASSERT(representation == kUnboxedInt32 || representation == kUnboxedUint32 ||
769+
representation == kUnboxedInt64);
770+
Value* right = Pop();
771+
Value* left = Pop();
772+
BinaryIntegerOpInstr* instr;
773+
switch (representation) {
774+
case kUnboxedInt32:
775+
instr = new (Z) BinaryInt32OpInstr(kind, left, right, GetNextDeoptId());
776+
break;
777+
case kUnboxedUint32:
778+
instr = new (Z) BinaryUint32OpInstr(kind, left, right, GetNextDeoptId());
779+
break;
780+
case kUnboxedInt64:
781+
instr = new (Z) BinaryInt64OpInstr(kind, left, right, GetNextDeoptId());
782+
break;
783+
default:
784+
UNREACHABLE();
785+
}
786+
if (is_truncating) {
787+
instr->mark_truncating();
788+
}
789+
Push(instr);
790+
return Fragment(instr);
791+
}
792+
765793
Fragment BaseFlowGraphBuilder::LoadFpRelativeSlot(intptr_t offset,
766794
CompileType result_type) {
767795
LoadIndexedUnsafeInstr* instr =

‎runtime/vm/compiler/frontend/base_flow_graph_builder.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,9 @@ class BaseFlowGraphBuilder {
247247
Fragment NullConstant();
248248
Fragment SmiRelationalOp(Token::Kind kind);
249249
Fragment SmiBinaryOp(Token::Kind op, bool is_truncating = false);
250+
Fragment BinaryIntegerOp(Token::Kind op,
251+
Representation representation,
252+
bool is_truncating = false);
250253
Fragment LoadFpRelativeSlot(intptr_t offset, CompileType result_type);
251254
Fragment StoreFpRelativeSlot(intptr_t offset);
252255
Fragment BranchIfTrue(TargetEntryInstr** then_entry,

0 commit comments

Comments
 (0)