Skip to content

Commit 6be50a4

Browse files
jensjohaCommit Queue
authored andcommitted
[analyzer] Fix signature calculation when merging yaml files with "error" keys
**TL;DR** In certain situations a new signature was created for every analyzer start causing more work than needed to be done and filling up the cache for no reason. The fix can cause the startup cost to improve significantly. **Details** When merging analysis options yaml files (when one file includes another), and both include data under "analyzer: errors:" we get inconsistent signatures because of ordering issues. This causes "old work" to be redone and more files to be written. This CL fixes the issue by not introducing a semi-random order when merging. For the contexts I have open, the following files gives (gave) a bad (inconsistent) signature: pkg/analyzer/test/analysis_options.yaml pkg/front_end/analysis_options.yaml pkg/analysis_server/test/analysis_options.yaml Looking at the time it takes from the first read/write until the last: Before: 37.148419 37.273016 36.807384 Now: 14.164655 15.251373 15.108677 While the difference is easily visible, for good measure: ``` Difference at 95.0% confidence -22.2347 +/- 1.0223 -59.9702% +/- 2.7573% (Student's t, pooled s = 0.45103) ``` And from the column "time" in top (note that I'm running from source so that this includes the initial dart compile of the analyzer): Before: 1:10.84 1:09.81 1:26.98 Now: 0:38.03 0:38.72 0:37.79 Again the difference is easily visible, but for good measure: ``` Difference at 95.0% confidence -37.6967 +/- 15.4529 -49.6815% +/- 20.3658% (Student's t, pooled s = 6.81767) ``` If clearing the cache and opening (waiting for writes to complete), closing and repeating, before this CL I got this with the contexts I have open: After 1st open: ``` $ du -chs ~/.dartServer/ 186M total ``` After the 2nd open: ``` $ du -chs ~/.dartServer/ 212M total ``` => +26MB After the 3rd open: ``` $ du -chs ~/.dartServer/ 247M total $ find ~/.dartServer/ | wc -l 25713 ``` => +35MB After the 4th open: ``` $ du -chs ~/.dartServer/ 272M total $ find ~/.dartServer/ | wc -l 27769 ``` => +25MB; +2056 files After the 5th open: ``` $ du -chs ~/.dartServer/ 297M total $ find ~/.dartServer/ | wc -l 29825 ``` => +25MB; +2056 files After the 6th open: ``` $ du -chs ~/.dartServer/ 321M total $ find ~/.dartServer/ | wc -l 31881 ``` => +24MB; +2056 files And we seem to have reached a point where we can guess the 7th if we want to. And with the fix in this CL: After 1st open: ``` $ du -chs ~/.dartServer/ 186M total $ find ~/.dartServer/ | wc -l 20816 ``` After 2nd: ``` $ du -chs ~/.dartServer/ 186M total $ find ~/.dartServer/ | wc -l 20816 ``` After the 3rd: ``` $ du -chs ~/.dartServer/ 186M total $ find ~/.dartServer/ | wc -l 20816 ``` And it seems that it's stable. We might also want to sort the strings as ordering (probably?) doesn't matter, but I'll leave that for another CL. Change-Id: Id06ca84e8b063bb31a261753f56620443f341a2e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/410700 Commit-Queue: Jens Johansen <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>
1 parent 3702cbe commit 6be50a4

File tree

3 files changed

+95
-8
lines changed

3 files changed

+95
-8
lines changed

pkg/analyzer/lib/src/util/yaml.dart

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
// for details. All rights reserved. Use of this source code is governed by a
33
// BSD-style license that can be found in the LICENSE file.
44

5-
import 'dart:collection';
6-
75
import 'package:source_span/source_span.dart';
86
import 'package:yaml/src/event.dart';
97
import 'package:yaml/yaml.dart';
@@ -33,8 +31,8 @@ class Merger {
3331
YamlNode merge(YamlNode o1, YamlNode o2) {
3432
// Handle promotion first.
3533
YamlMap listToMap(YamlList list) {
36-
Map<YamlNode, YamlNode> map =
37-
HashMap<YamlNode, YamlNode>(); // equals: _equals, hashCode: _hashCode
34+
// We use the default linked hash map so the ordering is the same every time.
35+
Map<YamlNode, YamlNode> map = {};
3836
ScalarEvent event =
3937
ScalarEvent(o1.span as FileSpan, 'true', ScalarStyle.PLAIN);
4038
for (var element in list.nodes) {
@@ -76,8 +74,8 @@ class Merger {
7674

7775
/// Merge maps (recursively).
7876
YamlMap mergeMap(YamlMap m1, YamlMap m2) {
79-
Map<YamlNode, YamlNode> merged =
80-
HashMap<YamlNode, YamlNode>(); // equals: _equals, hashCode: _hashCode
77+
// We use the default linked hash map so the ordering is the same every time.
78+
Map<YamlNode, YamlNode> merged = {};
8179
m1.nodeMap.forEach((k, v) {
8280
merged[k] = v;
8381
});

pkg/analyzer/test/src/options/analysis_options_test.dart

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import 'package:analyzer/file_system/memory_file_system.dart';
88
import 'package:analyzer/src/analysis_options/analysis_options_provider.dart';
99
import 'package:analyzer/src/dart/analysis/analysis_options.dart';
1010
import 'package:analyzer/src/error/codes.dart';
11+
import 'package:analyzer/src/file_system/file_system.dart';
12+
import 'package:analyzer/src/generated/source.dart';
1113
import 'package:test/test.dart';
1214
import 'package:test_reflective_loader/test_reflective_loader.dart';
1315

@@ -30,8 +32,8 @@ class AnalysisOptionsTest {
3032
AnalysisOptionsImpl parseOptions(String content) =>
3133
AnalysisOptionsImpl.fromYaml(
3234
optionsMap: optionsProvider.getOptionsFromString(content),
33-
file: resourceProvider.getFile(resourceProvider.convertPath(
34-
'/project/analysis_options.yaml')),
35+
file: resourceProvider.getFile(
36+
resourceProvider.convertPath('/project/analysis_options.yaml')),
3537
resourceProvider: resourceProvider,
3638
);
3739

@@ -414,4 +416,43 @@ code-style:
414416
''');
415417
expect(analysisOptions.codeStyleOptions.useFormatter, true);
416418
}
419+
420+
test_signature_on_merge() {
421+
var resourceProvider = MemoryResourceProvider();
422+
var sourceFactory = SourceFactory([ResourceUriResolver(resourceProvider)]);
423+
var optionsProvider = AnalysisOptionsProvider(sourceFactory);
424+
var otherOptions = resourceProvider.getFile(
425+
resourceProvider.convertPath("/project/analysis_options_helper.yaml"));
426+
otherOptions.writeAsStringSync('''
427+
analyzer:
428+
errors:
429+
a: warning
430+
b: ignore
431+
c: ignore
432+
''');
433+
var mainOptions = resourceProvider.getFile(
434+
resourceProvider.convertPath("/project/analysis_options.yaml"));
435+
mainOptions.writeAsStringSync('''
436+
include: analysis_options_helper.yaml
437+
analyzer:
438+
errors:
439+
d: ignore
440+
''');
441+
442+
var options = AnalysisOptionsImpl.fromYaml(
443+
optionsMap: optionsProvider.getOptionsFromFile(mainOptions),
444+
file: mainOptions,
445+
resourceProvider: resourceProvider,
446+
);
447+
var sig1 = options.signature;
448+
for (var i = 0; i < 100; i++) {
449+
var options2 = AnalysisOptionsImpl.fromYaml(
450+
optionsMap: optionsProvider.getOptionsFromFile(mainOptions),
451+
file: mainOptions,
452+
resourceProvider: resourceProvider,
453+
);
454+
var sig2 = options2.signature;
455+
expect(sig1, sig2);
456+
}
457+
}
417458
}

pkg/analyzer/test/src/util/yaml_test.dart

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,51 @@ main() {
9494
expect(merge(1, 'foo'), 'foo');
9595
expect(merge({'foo': 1}, 'foo'), 'foo');
9696
});
97+
98+
test('merged order directly from yaml', () {
99+
YamlMap loadYaml(String content) {
100+
return loadYamlNode(content) as YamlMap;
101+
}
102+
103+
var aYaml = """foo:
104+
a: bar""";
105+
var bYaml = """foo:
106+
b: baz""";
107+
108+
var merge1Value = mergeYamlMaps(loadYaml(bYaml), loadYaml(aYaml))
109+
.valueAt("foo")
110+
.toString();
111+
for (int i = 0; i < 100; i++) {
112+
var merge2Value = mergeYamlMaps(loadYaml(bYaml), loadYaml(aYaml))
113+
.valueAt("foo")
114+
.toString();
115+
expect(merge2Value, merge1Value);
116+
}
117+
});
118+
119+
test('merged w/promotion order directly from yaml', () {
120+
YamlMap loadYaml(String content) {
121+
return loadYamlNode(content) as YamlMap;
122+
}
123+
124+
var aYaml = """foo:
125+
- a
126+
- b
127+
- c
128+
- d""";
129+
var bYaml = """foo:
130+
b: true""";
131+
132+
var merge1Value = mergeYamlMaps(loadYaml(bYaml), loadYaml(aYaml))
133+
.valueAt("foo")
134+
.toString();
135+
for (int i = 0; i < 100; i++) {
136+
var merge2Value = mergeYamlMaps(loadYaml(bYaml), loadYaml(aYaml))
137+
.valueAt("foo")
138+
.toString();
139+
expect(merge2Value, merge1Value);
140+
}
141+
});
97142
});
98143
});
99144
}
@@ -103,6 +148,9 @@ final Merger merger = Merger();
103148
Object merge(Object o1, Object o2) =>
104149
merger.merge(wrap(o1), wrap(o2)).valueOrThrow;
105150

151+
YamlMap mergeYamlMaps(YamlMap defaults, YamlMap overrides) =>
152+
Merger().mergeMap(defaults, overrides);
153+
106154
YamlNode wrap(Object? value) {
107155
if (value is List) {
108156
var wrappedElements = value.map((e) => wrap(e)).toList();

0 commit comments

Comments
 (0)