From 26b1616422a0d1c9b1908a207d5ff33ffd178beb Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Thu, 8 Sep 2022 10:12:38 -0700 Subject: [PATCH] Use the new `IsolateRef.isolateGroupId` to speed up isolate deduping (#422) * Use isolateRef.isolateGroupId to skip some RPCs * Update the mock test * Fix analysis * Debugging the vm_service version of github CI * Remove debugging --- lib/src/collect.dart | 15 +- pubspec.yaml | 2 +- test/README.md | 9 + test/collect_coverage_mock_test.dart | 235 ++++++++++++++++----- test/collect_coverage_mock_test.mocks.dart | 2 +- 5 files changed, 199 insertions(+), 64 deletions(-) create mode 100644 test/README.md diff --git a/lib/src/collect.dart b/lib/src/collect.dart index b0df8436..f16f3f6d 100644 --- a/lib/src/collect.dart +++ b/lib/src/collect.dart @@ -123,6 +123,7 @@ Future> _getAllCoverage( final reportLines = _versionCheck(version, 3, 51); final branchCoverageSupported = _versionCheck(version, 3, 56); final libraryFilters = _versionCheck(version, 3, 57); + final fastIsoGroups = _versionCheck(version, 3, 61); if (branchCoverage && !branchCoverageSupported) { branchCoverage = false; stderr.writeln('Branch coverage was requested, but is not supported' @@ -138,16 +139,20 @@ Future> _getAllCoverage( // group, otherwise we'll double count the hits. final isolateOwnerGroup = {}; final coveredIsolateGroups = {}; - for (var isolateGroupRef in vm.isolateGroups!) { - final isolateGroup = await service.getIsolateGroup(isolateGroupRef.id!); - for (var isolateRef in isolateGroup.isolates!) { - isolateOwnerGroup[isolateRef.id!] = isolateGroupRef.id!; + if (!fastIsoGroups) { + for (var isolateGroupRef in vm.isolateGroups!) { + final isolateGroup = await service.getIsolateGroup(isolateGroupRef.id!); + for (var isolateRef in isolateGroup.isolates!) { + isolateOwnerGroup[isolateRef.id!] = isolateGroupRef.id!; + } } } for (var isolateRef in vm.isolates!) { if (isolateIds != null && !isolateIds.contains(isolateRef.id)) continue; - final isolateGroupId = isolateOwnerGroup[isolateRef.id]; + final isolateGroupId = fastIsoGroups + ? isolateRef.isolateGroupId + : isolateOwnerGroup[isolateRef.id]; if (isolateGroupId != null) { if (coveredIsolateGroups.contains(isolateGroupId)) continue; coveredIsolateGroups.add(isolateGroupId); diff --git a/pubspec.yaml b/pubspec.yaml index c40c9233..fc340ee8 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -13,7 +13,7 @@ dependencies: path: ^1.8.0 source_maps: ^0.10.10 stack_trace: ^1.10.0 - vm_service: ^9.2.0 + vm_service: ^9.4.0 dev_dependencies: benchmark_harness: ^2.2.0 diff --git a/test/README.md b/test/README.md new file mode 100644 index 00000000..8fa00df2 --- /dev/null +++ b/test/README.md @@ -0,0 +1,9 @@ +## Regenerating mocks + +Some of the tests use a mock VmService that is automatically generated by +Mockito. If the VmService changes, run this command in the root directory of +this repo to regenerate that mock: + +```bash +dart run build_runner build +``` diff --git a/test/collect_coverage_mock_test.dart b/test/collect_coverage_mock_test.dart index 162ed9d7..3e02e9eb 100644 --- a/test/collect_coverage_mock_test.dart +++ b/test/collect_coverage_mock_test.dart @@ -10,70 +10,51 @@ import 'package:vm_service/vm_service.dart'; import 'collect_coverage_mock_test.mocks.dart'; +@GenerateMocks([VmService]) SourceReportRange _range(int scriptIndex, SourceReportCoverage coverage) => - SourceReportRange( - scriptIndex: scriptIndex, - startPos: null, - endPos: null, - compiled: null, - error: null, - coverage: coverage, - possibleBreakpoints: null, - branchCoverage: null, - ); - -Script _script(List> tokenPosTable) => Script( - uri: null, - library: null, - id: '', - lineOffset: null, - columnOffset: null, - source: null, - tokenPosTable: tokenPosTable, - ); - -MockVmService _mockService(int majorVersion, int minorVersion) { + SourceReportRange(scriptIndex: scriptIndex, coverage: coverage); + +Script _script(List> tokenPosTable) => + Script(id: 'script', tokenPosTable: tokenPosTable); + +IsolateRef _isoRef(String id, String isoGroupId) => + IsolateRef(id: id, isolateGroupId: isoGroupId); + +IsolateGroupRef _isoGroupRef(String id) => IsolateGroupRef(id: id); + +IsolateGroup _isoGroup(String id, List isolates) => + IsolateGroup(id: id, isolates: isolates); + +MockVmService _mockService( + int majorVersion, + int minorVersion, { + Map> isolateGroups = const { + 'isolateGroup': ['isolate'], + }, +}) { final service = MockVmService(); - final isoRef = IsolateRef( - id: 'isolate', - number: null, - name: null, - isSystemIsolate: null, - ); - final isoGroupRef = IsolateGroupRef( - id: 'isolateGroup', - number: null, - name: null, - isSystemIsolateGroup: null, - ); - when(service.getVM()).thenAnswer((_) async => VM( - name: null, - architectureBits: null, - hostCPU: null, - operatingSystem: null, - targetCPU: null, - version: null, - pid: null, - startTime: null, - isolates: [isoRef], - isolateGroups: [isoGroupRef], - systemIsolates: null, - systemIsolateGroups: null, - )); - when(service.getIsolateGroup('isolateGroup')) - .thenAnswer((_) async => IsolateGroup( - id: 'isolateGroup', - number: null, - name: null, - isSystemIsolateGroup: null, - isolates: [isoRef], - )); + final isoRefs = []; + final isoGroupRefs = []; + final isoGroups = []; + for (final group in isolateGroups.entries) { + isoGroupRefs.add(_isoGroupRef(group.key)); + final isosOfGroup = []; + for (final isoId in group.value) { + isosOfGroup.add(_isoRef(isoId, group.key)); + } + isoGroups.add(_isoGroup(group.key, isosOfGroup)); + isoRefs.addAll(isosOfGroup); + } + when(service.getVM()).thenAnswer( + (_) async => VM(isolates: isoRefs, isolateGroups: isoGroupRefs)); + for (final group in isoGroups) { + when(service.getIsolateGroup(group.id)).thenAnswer((_) async => group); + } when(service.getVersion()).thenAnswer( (_) async => Version(major: majorVersion, minor: minorVersion)); return service; } -@GenerateMocks([VmService]) void main() { group('Mock VM Service', () { test('Collect coverage', () async { @@ -251,5 +232,145 @@ void main() { expect(result.length, 1); expect(result['package:foo/foo.dart']?.lineHits, {12: 1, 47: 0}); }); + + test('Collect coverage, old isolate group deduping', () async { + final service = _mockService(3, 60, isolateGroups: { + 'isolateGroupA': ['isolate1', 'isolate2'], + 'isolateGroupB': ['isolate3'], + }); + when(service.getSourceReport('isolate1', ['Coverage'], + forceCompile: true, reportLines: true)) + .thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [12], + misses: [47], + ), + ), + _range( + 1, + SourceReportCoverage( + hits: [95], + misses: [52], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:foo/foo.dart', + id: 'foo', + ), + ScriptRef( + uri: 'package:bar/bar.dart', + id: 'bar', + ), + ], + )); + when(service.getSourceReport('isolate3', ['Coverage'], + forceCompile: true, reportLines: true)) + .thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [34], + misses: [61], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:baz/baz.dart', + id: 'baz', + ), + ], + )); + + final jsonResult = await collect(Uri(), false, false, false, null, + serviceOverrideForTesting: service); + final result = await HitMap.parseJson( + jsonResult['coverage'] as List>); + + expect(result.length, 3); + expect(result['package:foo/foo.dart']?.lineHits, {12: 1, 47: 0}); + expect(result['package:bar/bar.dart']?.lineHits, {95: 1, 52: 0}); + expect(result['package:baz/baz.dart']?.lineHits, {34: 1, 61: 0}); + verifyNever(service.getSourceReport('isolate2', ['Coverage'], + forceCompile: true, reportLines: true)); + verify(service.getIsolateGroup('isolateGroupA')); + verify(service.getIsolateGroup('isolateGroupB')); + }); + + test('Collect coverage, fast isolate group deduping', () async { + final service = _mockService(3, 61, isolateGroups: { + 'isolateGroupA': ['isolate1', 'isolate2'], + 'isolateGroupB': ['isolate3'], + }); + when(service.getSourceReport('isolate1', ['Coverage'], + forceCompile: true, reportLines: true)) + .thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [12], + misses: [47], + ), + ), + _range( + 1, + SourceReportCoverage( + hits: [95], + misses: [52], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:foo/foo.dart', + id: 'foo', + ), + ScriptRef( + uri: 'package:bar/bar.dart', + id: 'bar', + ), + ], + )); + when(service.getSourceReport('isolate3', ['Coverage'], + forceCompile: true, reportLines: true)) + .thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [34], + misses: [61], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:baz/baz.dart', + id: 'baz', + ), + ], + )); + + final jsonResult = await collect(Uri(), false, false, false, null, + serviceOverrideForTesting: service); + final result = await HitMap.parseJson( + jsonResult['coverage'] as List>); + + expect(result.length, 3); + expect(result['package:foo/foo.dart']?.lineHits, {12: 1, 47: 0}); + expect(result['package:bar/bar.dart']?.lineHits, {95: 1, 52: 0}); + expect(result['package:baz/baz.dart']?.lineHits, {34: 1, 61: 0}); + verifyNever(service.getSourceReport('isolate2', ['Coverage'], + forceCompile: true, reportLines: true)); + verifyNever(service.getIsolateGroup('isolateGroupA')); + verifyNever(service.getIsolateGroup('isolateGroupB')); + }); }); } diff --git a/test/collect_coverage_mock_test.mocks.dart b/test/collect_coverage_mock_test.mocks.dart index 0338186f..4ee09711 100644 --- a/test/collect_coverage_mock_test.mocks.dart +++ b/test/collect_coverage_mock_test.mocks.dart @@ -1,4 +1,4 @@ -// Mocks generated by Mockito 5.1.0 from annotations +// Mocks generated by Mockito 5.2.0 from annotations // in coverage/test/collect_coverage_mock_test.dart. // Do not manually edit this file.