[vm] Fix vm_snapshot_analysis logic and tests
* When looking for Code node owner simply always check for synthetic
owner (referenced through ':owner_' property) first. If it is present
then it means 'owner_' property is invalid anyway. 25fd0200ef1 changed
how WSRs are written into snapshot (instead of writing WSRs we are
simply writing SMI CIDs) which broke previous logic.
* When building ProgramInfoNode for a Class node accommodate for the
existence of classes which don't have 'library_' property (e.g. base
objects like void and dynamic). Additionally, adjust how these base
objects are written into the snapshot profile so that we preserve
their names.
* Tweak tests diff and diff-collapse tests to pass, they were failing
because prologue removal compiler change made some of the diffs so
tiny that they are ignored by threshold filtering.
* Add two more tests: one that verifies that tracings flags don't have
any impact on the snapshot size and one that checks we correct
attribute instructions size to the function node from which these
instructions originated. This last test is added in anticipation of
Code object removal to make sure that vm_snapshot_analysis continues
to work correctly.
* Adjust sorting rules for Code cluster to make it more stable between
slightly different programs to stabilize output of
--print-instructions-sizes-to (depends on the sorting algorithm used
in libc, the test was failing on Mac but not on Linux).
These fixes should unblock relanding of b6dc4dad4da
Fixes #44507
Fixes flutter/flutter#76313
TEST=pkg/vm_snapshot_analysis/test/instruction_sizes_test.dart
Cq-Include-Trybots: luci.dart.try:pkg-linux-release-try,pkg-win-release-try,pkg-mac-release-try
Change-Id: Iee0221dd3a507cb4f36d530404693ebfab3b7bf2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185826
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
diff --git a/pkg/vm_snapshot_analysis/lib/v8_profile.dart b/pkg/vm_snapshot_analysis/lib/v8_profile.dart
index 0c824f2..404fe66 100644
--- a/pkg/vm_snapshot_analysis/lib/v8_profile.dart
+++ b/pkg/vm_snapshot_analysis/lib/v8_profile.dart
@@ -398,11 +398,8 @@
ProgramInfoNode createInfoNodeFor(Node node) {
switch (node.type) {
case 'Code':
- var owner = node['owner_'];
+ var owner = node[':owner_'] ?? node['owner_'];
if (owner.type != 'Type') {
- if (owner.type == 'WeakSerializationReference') {
- owner = node[':owner_'];
- }
final ownerNode =
owner.type == 'Null' ? program.stubs : getInfoNodeFor(owner);
if (owner.type == 'Function') {
@@ -437,13 +434,15 @@
return getInfoNodeFor(node['patched_class_']);
case 'Class':
+ // Default to root node. Some builtin classes (void, dynamic) don't have
+ // any information about their library written out.
+ var ownerNode = program.root;
if (node['library_'] != null) {
- return makeInfoNode(node.index,
- name: node.name,
- parent: getInfoNodeFor(node['library_']) ?? program.root,
- type: NodeType.classNode);
+ ownerNode = getInfoNodeFor(node['library_']) ?? ownerNode;
}
- break;
+
+ return makeInfoNode(node.index,
+ name: node.name, parent: ownerNode, type: NodeType.classNode);
case 'Library':
// Create fake owner node for the package which contains this library.
diff --git a/pkg/vm_snapshot_analysis/test/instruction_sizes_test.dart b/pkg/vm_snapshot_analysis/test/instruction_sizes_test.dart
index fd4cdd8..ddb2ac4 100644
--- a/pkg/vm_snapshot_analysis/test/instruction_sizes_test.dart
+++ b/pkg/vm_snapshot_analysis/test/instruction_sizes_test.dart
@@ -24,9 +24,9 @@
@pragma('vm:never-inline')
dynamic makeSomeClosures() {
return [
- () => const K(0),
- () => const K(1),
- () => 11,
+ () => [const K(0)],
+ () => [const K(1)],
+ () => [11],
];
}
@@ -77,10 +77,10 @@
@pragma('vm:never-inline')
dynamic makeSomeClosures() {
return [
- () => const K(0),
- () => const K(1),
- () => 11,
- () => {}, // modified
+ () => [const K(0)],
+ () => [const K(1)],
+ () => [11],
+ () => [const K(2)], // modified
];
}
@@ -130,7 +130,7 @@
@pragma('vm:never-inline')
dynamic makeSomeClosures() {
return [
- () => const K(0),
+ () => [const K(0)],
];
}
@@ -202,10 +202,24 @@
// If we are not running from the prebuilt SDK then this test does nothing.
return;
}
+ group('no-size-impact', () {
+ for (var flag in [printInstructionSizesFlag, writeV8ProfileFlag]) {
+ test(flag, () async {
+ await withFlagImpl(testSource, null, (baseline) async {
+ await withFlagImpl(testSource, flag, (snapshotWithFlag) async {
+ final sizeBaseline = File(baseline.outputBinary).statSync().size;
+ final sizeWithFlag =
+ File(snapshotWithFlag.outputBinary).statSync().size;
+ expect(sizeWithFlag - sizeBaseline, equals(0));
+ });
+ });
+ });
+ }
+ });
group('instruction-sizes', () {
test('basic-parsing', () async {
- await withSymbolSizes('basic-parsing', testSource, (sizesJson) async {
+ await withSymbolSizes(testSource, (sizesJson) async {
final json = await loadJson(File(sizesJson));
final symbols = instruction_sizes.fromJson(json);
expect(symbols, isNotNull,
@@ -278,8 +292,7 @@
});
test('program-info-from-sizes', () async {
- await withSymbolSizes('program-info-from-sizes', testSource,
- (sizesJson) async {
+ await withSymbolSizes(testSource, (sizesJson) async {
final json = await loadJson(File(sizesJson));
final info = loadProgramInfoFromJson(json);
expect(info.root.children, contains('dart:core'));
@@ -327,7 +340,7 @@
});
test('histograms', () async {
- await withSymbolSizes('histograms', testSource, (sizesJson) async {
+ await withSymbolSizes(testSource, (sizesJson) async {
final json = await loadJson(File(sizesJson));
final info = loadProgramInfoFromJson(json);
final bySymbol = computeHistogram(info, HistogramType.bySymbol);
@@ -379,9 +392,8 @@
});
test('diff', () async {
- await withSymbolSizes('diff-1', testSource, (sizesJson) async {
- await withSymbolSizes('diff-2', testSourceModified,
- (modifiedSizesJson) async {
+ await withSymbolSizes(testSource, (sizesJson) async {
+ await withSymbolSizes(testSourceModified, (modifiedSizesJson) async {
final infoJson = await loadJson(File(sizesJson));
final info = loadProgramInfoFromJson(infoJson);
final modifiedJson = await loadJson(File(modifiedSizesJson));
@@ -401,7 +413,7 @@
'makeSomeClosures': {
'#type': 'function',
'#size': greaterThan(0), // We added code here.
- '<anonymous closure @180>': {
+ '<anonymous closure @186>': {
'#type': 'function',
'#size': greaterThan(0),
},
@@ -426,9 +438,8 @@
});
test('diff-collapsed', () async {
- await withSymbolSizes('diff-collapsed-1', testSource, (sizesJson) async {
- await withSymbolSizes('diff-collapsed-2', testSourceModified2,
- (modifiedSizesJson) async {
+ await withSymbolSizes(testSource, (sizesJson) async {
+ await withSymbolSizes(testSourceModified2, (modifiedSizesJson) async {
final json = await loadJson(File(sizesJson));
final info =
loadProgramInfoFromJson(json, collapseAnonymousClosures: true);
@@ -436,11 +447,17 @@
final modifiedInfo = loadProgramInfoFromJson(modifiedJson,
collapseAnonymousClosures: true);
final diff = computeDiff(info, modifiedInfo);
-
expect(
diffToJson(diff),
equals({
'#type': 'library',
+ '@stubs': {
+ '#type': 'library',
+ 'Type Test Type: int*': {
+ '#type': 'function',
+ '#size': lessThan(0),
+ },
+ },
'package:input': {
'#type': 'package',
'package:input/input.dart': {
@@ -466,8 +483,7 @@
group('v8-profile', () {
test('program-info-from-profile', () async {
- await withV8Profile('program-info-from-profile', testSource,
- (profileJson) async {
+ await withV8Profile(testSource, (profileJson) async {
final infoJson = await loadJson(File(profileJson));
final info = loadProgramInfoFromJson(infoJson);
expect(info.root.children, contains('dart:core'));
@@ -525,8 +541,42 @@
});
});
+ // This test verifies that we are able to attribute instructions to the
+ // function they originated from.
+ test('instruction ownership is preserved', () async {
+ await withV8Profile(testSource, (profileJson) async {
+ await withSymbolSizes(testSource, (sizesJson) async {
+ final fromProfile =
+ loadProgramInfoFromJson(await loadJson(File(profileJson)));
+ final fromSymbolSizes = loadProgramInfoFromJson(
+ await loadJson(File(sizesJson)),
+ collapseAnonymousClosures: true);
+ final functionNode = fromProfile.lookup([
+ 'package:input',
+ 'package:input/input.dart',
+ '::',
+ 'makeSomeClosures'
+ ]);
+ final codeNode = fromProfile.snapshotInfo.snapshot.nodes.firstWhere(
+ (n) =>
+ n.type == 'Code' &&
+ fromProfile.snapshotInfo.ownerOf(n) == functionNode &&
+ n.name.contains('makeSomeClosures'));
+ expect(codeNode['<instructions>'], isNotNull);
+ final instructionsSize = codeNode['<instructions>'].selfSize;
+ final symbolSize = fromSymbolSizes.lookup([
+ 'package:input',
+ 'package:input/input.dart',
+ '',
+ 'makeSomeClosures'
+ ]).size;
+ expect(instructionsSize - symbolSize, equals(0));
+ });
+ });
+ });
+
test('histograms', () async {
- await withV8Profile('histograms', testSource, (sizesJson) async {
+ await withV8Profile(testSource, (sizesJson) async {
final infoJson = await loadJson(File(sizesJson));
final info = loadProgramInfoFromJson(infoJson);
final bySymbol = computeHistogram(info, HistogramType.bySymbol);
@@ -578,9 +628,8 @@
});
test('diff', () async {
- await withV8Profile('diff-1', testSource, (profileJson) async {
- await withV8Profile('diff-2', testSourceModified,
- (modifiedProfileJson) async {
+ await withV8Profile(testSource, (profileJson) async {
+ await withV8Profile(testSourceModified, (modifiedProfileJson) async {
final infoJson = await loadJson(File(profileJson));
final info = loadProgramInfoFromJson(infoJson);
final modifiedJson = await loadJson(File(modifiedProfileJson));
@@ -599,7 +648,7 @@
'makeSomeClosures': {
'#type': 'function',
'#size': greaterThan(0),
- '<anonymous closure @180>': {
+ '<anonymous closure @186>': {
'#type': 'function',
'#size': greaterThan(0),
},
@@ -624,9 +673,8 @@
});
test('diff-collapsed', () async {
- await withV8Profile('diff-collapsed-1', testSource, (profileJson) async {
- await withV8Profile('diff-collapsed-2', testSourceModified2,
- (modifiedProfileJson) async {
+ await withV8Profile(testSource, (profileJson) async {
+ await withV8Profile(testSourceModified2, (modifiedProfileJson) async {
final infoJson = await loadJson(File(profileJson));
final info = loadProgramInfoFromJson(infoJson,
collapseAnonymousClosures: true);
@@ -646,7 +694,6 @@
'#type': 'class',
'makeSomeClosures': {
'#type': 'function',
- '#size': lessThan(0),
'<anonymous closure>': {
'#type': 'function',
'#size': lessThan(0),
@@ -661,7 +708,7 @@
});
test('treemap', () async {
- await withV8Profile('treemap', testSource, (profileJson) async {
+ await withV8Profile(testSource, (profileJson) async {
final infoJson = await loadJson(File(profileJson));
final info = await loadProgramInfoFromJson(infoJson,
collapseAnonymousClosures: true);
@@ -695,8 +742,7 @@
});
test('dominators', () async {
- await withV8Profile('dominators', chainOfStaticCalls,
- (profileJson) async {
+ await withV8Profile(chainOfStaticCalls, (profileJson) async {
// Note: computing dominators also verifies that we don't have
// unreachable nodes in the snapshot.
final infoJson = await loadJson(File(profileJson));
@@ -709,13 +755,17 @@
});
}
-Future withSymbolSizes(String prefix, Map<String, String> source,
- Future Function(String sizesJson) f) =>
- withFlag(prefix, source, '--print_instructions_sizes_to', f);
+final printInstructionSizesFlag = '--print_instructions_sizes_to';
-Future withV8Profile(String prefix, Map<String, String> source,
- Future Function(String sizesJson) f) =>
- withFlag(prefix, source, '--write_v8_snapshot_profile_to', f);
+Future withSymbolSizes(
+ Map<String, String> source, Future Function(String sizesJson) f) =>
+ withFlag(source, printInstructionSizesFlag, f);
+
+final writeV8ProfileFlag = '--write_v8_snapshot_profile_to';
+
+Future withV8Profile(
+ Map<String, String> source, Future Function(String sizesJson) f) =>
+ withFlag(source, writeV8ProfileFlag, f);
// On Windows there is some issue with interpreting entry point URI as a package URI
// it instead gets interpreted as a file URI - which breaks comparison. So we
diff --git a/pkg/vm_snapshot_analysis/test/precompiler_trace_test.dart b/pkg/vm_snapshot_analysis/test/precompiler_trace_test.dart
index c4ad546..2392171 100644
--- a/pkg/vm_snapshot_analysis/test/precompiler_trace_test.dart
+++ b/pkg/vm_snapshot_analysis/test/precompiler_trace_test.dart
@@ -69,8 +69,7 @@
group('precompiler-trace', () {
test('basic-parsing', () async {
- await withFlag('basic-parsing', testSource, '--trace_precompiler_to',
- (json) async {
+ await withFlag(testSource, '--trace_precompiler_to', (json) async {
final jsonRaw = await loadJson(File(json));
final callGraph = loadTrace(jsonRaw);
callGraph.computeDominators();
@@ -102,9 +101,7 @@
});
test('collapse-by-package', () async {
- await withFlag(
- 'collapse-by-package', testSource, '--trace_precompiler_to',
- (json) async {
+ await withFlag(testSource, '--trace_precompiler_to', (json) async {
final jsonRaw = await loadJson(File(json));
final callGraph = loadTrace(jsonRaw).collapse(NodeType.packageNode);
diff --git a/pkg/vm_snapshot_analysis/test/utils.dart b/pkg/vm_snapshot_analysis/test/utils.dart
index acdf42f..bdc14e7 100644
--- a/pkg/vm_snapshot_analysis/test/utils.dart
+++ b/pkg/vm_snapshot_analysis/test/utils.dart
@@ -20,11 +20,25 @@
return path.canonicalize(dart2native);
}();
-Future withFlag(String prefix, Map<String, String> source, String flag,
- Future Function(String sizesJson) f) {
- return withTempDir(prefix, (dir) async {
- final outputBinary = path.join(dir, 'output.exe');
- final sizesJson = path.join(dir, 'sizes.json');
+class AotSnapshot {
+ final String outputBinary;
+ final String sizesJson;
+
+ AotSnapshot({this.outputBinary, this.sizesJson});
+}
+
+Future withFlag(
+ Map<String, String> source, String flag, Future Function(String) f) {
+ return withFlagImpl(source, flag, (info) => f(info.sizesJson));
+}
+
+Future withFlagImpl(
+ Map<String, String> source, String flag, Future Function(AotSnapshot) f) {
+ return withTempDir((dir) async {
+ final snapshot = AotSnapshot(
+ outputBinary: path.join(dir, 'output.exe'),
+ sizesJson: path.join(dir, 'sizes.json'),
+ );
final packages = path.join(dir, '.packages');
final mainDart = path.join(dir, 'main.dart');
@@ -41,12 +55,17 @@
void main(List<String> args) => input.main(args);
''');
+ final extraGenSnapshotOptions = [
+ '--dwarf-stack-traces',
+ if (flag != null) '$flag=${snapshot.sizesJson}',
+ ];
+
// Compile input.dart to native and output instruction sizes.
final result = await Process.run(dart2native, [
'-o',
- outputBinary,
+ snapshot.outputBinary,
'--packages=$packages',
- '--extra-gen-snapshot-options=--dwarf-stack-traces,$flag=$sizesJson',
+ '--extra-gen-snapshot-options=${extraGenSnapshotOptions.join(',')}',
mainDart,
]);
@@ -56,18 +75,20 @@
stdout: ${result.stdout}
stderr: ${result.stderr}
''');
- expect(File(outputBinary).existsSync(), isTrue,
+ expect(File(snapshot.outputBinary).existsSync(), isTrue,
reason: 'Output binary exists');
- expect(File(sizesJson).existsSync(), isTrue,
- reason: 'Instruction sizes output exists');
+ if (flag != null) {
+ expect(File(snapshot.sizesJson).existsSync(), isTrue,
+ reason: 'Instruction sizes output exists');
+ }
- await f(sizesJson);
+ await f(snapshot);
});
}
-Future withTempDir(String prefix, Future Function(String dir) f) async {
+Future withTempDir(Future Function(String dir) f) async {
final tempDir =
- Directory.systemTemp.createTempSync('instruction-sizes-test-${prefix}');
+ Directory.systemTemp.createTempSync('instruction-sizes-test-');
try {
await f(tempDir.path);
} finally {
diff --git a/runtime/vm/clustered_snapshot.cc b/runtime/vm/clustered_snapshot.cc
index 1849a38..1661602 100644
--- a/runtime/vm/clustered_snapshot.cc
+++ b/runtime/vm/clustered_snapshot.cc
@@ -1499,18 +1499,28 @@
struct CodeOrderInfo {
CodePtr code;
intptr_t order;
+ intptr_t original_index;
};
+ // We sort code objects in such a way that code objects with the same
+ // instructions are grouped together. To make sorting more stable between
+ // similar programs we also sort them further by their original indices -
+ // this helps to stabilize output of --print-instructions-sizes-to which uses
+ // the name of the first code object (among those pointing to the same
+ // instruction objects).
static int CompareCodeOrderInfo(CodeOrderInfo const* a,
CodeOrderInfo const* b) {
if (a->order < b->order) return -1;
if (a->order > b->order) return 1;
+ if (a->original_index < b->original_index) return -1;
+ if (a->original_index > b->original_index) return 1;
return 0;
}
static void Insert(GrowableArray<CodeOrderInfo>* order_list,
IntMap<intptr_t>* order_map,
- CodePtr code) {
+ CodePtr code,
+ intptr_t original_index) {
InstructionsPtr instr = code->untag()->instructions_;
intptr_t key = static_cast<intptr_t>(instr);
intptr_t order;
@@ -1523,6 +1533,7 @@
CodeOrderInfo info;
info.code = code;
info.order = order;
+ info.original_index = original_index;
order_list->Add(info);
}
@@ -1530,7 +1541,7 @@
GrowableArray<CodeOrderInfo> order_list;
IntMap<intptr_t> order_map;
for (intptr_t i = 0; i < codes->length(); i++) {
- Insert(&order_list, &order_map, (*codes)[i]);
+ Insert(&order_list, &order_map, (*codes)[i], i);
}
order_list.Sort(CompareCodeOrderInfo);
ASSERT(order_list.length() == codes->length());
@@ -1543,7 +1554,7 @@
GrowableArray<CodeOrderInfo> order_list;
IntMap<intptr_t> order_map;
for (intptr_t i = 0; i < codes->length(); i++) {
- Insert(&order_list, &order_map, (*codes)[i]->ptr());
+ Insert(&order_list, &order_map, (*codes)[i]->ptr(), i);
}
order_list.Sort(CompareCodeOrderInfo);
ASSERT(order_list.length() == codes->length());
@@ -5030,11 +5041,14 @@
// Error, CallSiteData has no class object.
if (cid != kErrorCid && cid != kCallSiteDataCid) {
ASSERT(table->HasValidClassAt(cid));
- s->AddBaseObject(table->At(cid), "Class");
+ s->AddBaseObject(
+ table->At(cid), "Class",
+ Class::Handle(table->At(cid))
+ .NameCString(Object::NameVisibility::kInternalName));
}
}
- s->AddBaseObject(table->At(kDynamicCid), "Class");
- s->AddBaseObject(table->At(kVoidCid), "Class");
+ s->AddBaseObject(table->At(kDynamicCid), "Class", "dynamic");
+ s->AddBaseObject(table->At(kVoidCid), "Class", "void");
if (!Snapshot::IncludesCode(s->kind())) {
for (intptr_t i = 0; i < StubCode::NumEntries(); i++) {
