From 011068f8242b1f9f6ce9dcd96551591bdd585b25 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Thu, 26 Aug 2021 18:42:58 +0000 Subject: [PATCH] [ VM / Service ] Add reportLines flag to getSourceReport RPC Every known user of the coverage report just wants the line numbers. At the moment they have to do a second RPC to get the Script object, so they can translate the token positions into line numbers. Slower test times with coverage are usually caused by the extra time it takes to run the RPCs. So reporting the line number directly will halve the time it takes to get coverage, for most users. Bug: https://github.com/flutter/flutter/issues/86722 Change-Id: I7b8d436669713ebc7b7096790a02593b9cb94dda TEST=CI Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/211081 Commit-Queue: Liam Appelbe Reviewed-by: Ben Konyi --- pkg/vm_service/CHANGELOG.md | 4 + pkg/vm_service/java/version.properties | 2 +- pkg/vm_service/lib/src/vm_service.dart | 27 +++-- pkg/vm_service/pubspec.yaml | 2 +- .../test/coverage_leaf_function_test.dart | 111 ++++++++++-------- .../tests/service/get_version_rpc_test.dart | 2 +- .../tests/service_2/get_version_rpc_test.dart | 2 +- runtime/vm/service.cc | 5 +- runtime/vm/service.h | 2 +- runtime/vm/service/service.md | 28 +++-- runtime/vm/source_report.cc | 38 ++++-- runtime/vm/source_report.h | 7 +- 12 files changed, 147 insertions(+), 83 deletions(-) diff --git a/pkg/vm_service/CHANGELOG.md b/pkg/vm_service/CHANGELOG.md index 3c1a96476eb..6b3803ca263 100644 --- a/pkg/vm_service/CHANGELOG.md +++ b/pkg/vm_service/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 7.3.0 +- Update to version `3.51` of the spec. +- Added optional `reportLines` parameter to `getSourceReport` RPC. + ## 7.1.1 - Update to version `3.48` of the spec. - Added `shows` and `hides` properties to `LibraryDependency`. diff --git a/pkg/vm_service/java/version.properties b/pkg/vm_service/java/version.properties index 5e787edb97a..383674e7a7f 100644 --- a/pkg/vm_service/java/version.properties +++ b/pkg/vm_service/java/version.properties @@ -1 +1 @@ -version=3.49 +version=3.51 diff --git a/pkg/vm_service/lib/src/vm_service.dart b/pkg/vm_service/lib/src/vm_service.dart index 77ce9047537..277b4637b53 100644 --- a/pkg/vm_service/lib/src/vm_service.dart +++ b/pkg/vm_service/lib/src/vm_service.dart @@ -26,7 +26,7 @@ export 'snapshot_graph.dart' HeapSnapshotObjectNoData, HeapSnapshotObjectNullData; -const String vmServiceVersion = '3.49.0'; +const String vmServiceVersion = '3.51.0'; /// @optional const String optional = 'optional'; @@ -817,6 +817,13 @@ abstract class VmServiceInterface { /// compilation error, which could terminate the running Dart program. If this /// parameter is not provided, it is considered to have the value `false`. /// + /// The `reportLines` parameter changes the token positions in + /// `SourceReportRange.possibleBreakpoints` and `SourceReportCoverage` to be + /// line numbers. This is designed to reduce the number of RPCs that need to + /// be performed in the case that the client is only interested in line + /// numbers. If this parameter is not provided, it is considered to have the + /// value `false`. + /// /// If `isolateId` refers to an isolate which has exited, then the `Collected` /// [Sentinel] is returned. /// @@ -831,6 +838,7 @@ abstract class VmServiceInterface { int? tokenPos, int? endTokenPos, bool? forceCompile, + bool? reportLines, }); /// The `getVersion` RPC is used to determine what version of the Service @@ -1429,6 +1437,7 @@ class VmServerConnection { tokenPos: params['tokenPos'], endTokenPos: params['endTokenPos'], forceCompile: params['forceCompile'], + reportLines: params['reportLines'], ); break; case 'getVersion': @@ -1929,6 +1938,7 @@ class VmService implements VmServiceInterface { int? tokenPos, int? endTokenPos, bool? forceCompile, + bool? reportLines, }) => _call('getSourceReport', { 'isolateId': isolateId, @@ -1937,6 +1947,7 @@ class VmService implements VmServiceInterface { if (tokenPos != null) 'tokenPos': tokenPos, if (endTokenPos != null) 'endTokenPos': endTokenPos, if (forceCompile != null) 'forceCompile': forceCompile, + if (reportLines != null) 'reportLines': reportLines, }); @override @@ -7287,12 +7298,12 @@ class SourceReportCoverage { static SourceReportCoverage? parse(Map? json) => json == null ? null : SourceReportCoverage._fromJson(json); - /// A list of token positions in a SourceReportRange which have been executed. - /// The list is sorted. + /// A list of token positions (or line numbers if reportLines was enabled) in + /// a SourceReportRange which have been executed. The list is sorted. List? hits; - /// A list of token positions in a SourceReportRange which have not been - /// executed. The list is sorted. + /// A list of token positions (or line numbers if reportLines was enabled) in + /// a SourceReportRange which have not been executed. The list is sorted. List? misses; SourceReportCoverage({ @@ -7352,9 +7363,9 @@ class SourceReportRange { SourceReportCoverage? coverage; /// Possible breakpoint information for this range, represented as a sorted - /// list of token positions. Provided only when the when the - /// PossibleBreakpoint report has been requested and the range has been - /// compiled. + /// list of token positions (or line numbers if reportLines was enabled). + /// Provided only when the when the PossibleBreakpoint report has been + /// requested and the range has been compiled. @optional List? possibleBreakpoints; diff --git a/pkg/vm_service/pubspec.yaml b/pkg/vm_service/pubspec.yaml index 5aad6b63552..85b7cadf275 100644 --- a/pkg/vm_service/pubspec.yaml +++ b/pkg/vm_service/pubspec.yaml @@ -3,7 +3,7 @@ description: >- A library to communicate with a service implementing the Dart VM service protocol. -version: 7.1.1 +version: 7.3.0 homepage: https://github.com/dart-lang/sdk/tree/master/pkg/vm_service diff --git a/pkg/vm_service/test/coverage_leaf_function_test.dart b/pkg/vm_service/test/coverage_leaf_function_test.dart index fd15551c95b..36b9a889d18 100644 --- a/pkg/vm_service/test/coverage_leaf_function_test.dart +++ b/pkg/vm_service/test/coverage_leaf_function_test.dart @@ -27,9 +27,9 @@ bool allRangesCompiled(coverage) { return true; } -var tests = [ - hasStoppedAtBreakpoint, - (VmService service, IsolateRef isolateRef) async { +IsolateTest coverageTest(Map expectedRange, + {required bool reportLines}) { + return (VmService service, IsolateRef isolateRef) async { final isolateId = isolateRef.id!; final isolate = await service.getIsolate(isolateId); final stack = await service.getStack(isolateId); @@ -43,8 +43,31 @@ var tests = [ FuncRef funcRef = root.functions!.singleWhere((f) => f.name == 'leafFunction'); Func func = await service.getObject(isolateId, funcRef.id!) as Func; + final location = func.location!; - final expectedRange = { + final report = await service.getSourceReport( + isolateId, + [SourceReportKind.kCoverage], + scriptId: location.script!.id, + tokenPos: location.tokenPos, + endTokenPos: location.endTokenPos, + forceCompile: true, + reportLines: reportLines, + ); + expect(report.ranges!.length, 1); + expect(report.ranges![0].toJson(), expectedRange); + expect(report.scripts!.length, 1); + expect( + report.scripts![0].uri, + endsWith('coverage_leaf_function_test.dart'), + ); + }; +} + +var tests = [ + hasStoppedAtBreakpoint, + coverageTest( + { 'scriptIndex': 0, 'startPos': 397, 'endPos': 447, @@ -53,39 +76,26 @@ var tests = [ 'hits': [], 'misses': [397] } - }; - final location = func.location!; - - final report = await service.getSourceReport( - isolateId, [SourceReportKind.kCoverage], - scriptId: location.script!.id, - tokenPos: location.tokenPos, - endTokenPos: location.endTokenPos, - forceCompile: true); - expect(report.ranges!.length, 1); - expect(report.ranges![0].toJson(), expectedRange); - expect(report.scripts!.length, 1); - expect( - report.scripts![0].uri, endsWith('coverage_leaf_function_test.dart')); - }, + }, + reportLines: false, + ), + coverageTest( + { + 'scriptIndex': 0, + 'startPos': 397, + 'endPos': 447, + 'compiled': true, + 'coverage': { + 'hits': [], + 'misses': [11] + } + }, + reportLines: true, + ), resumeIsolate, hasStoppedAtBreakpoint, - (VmService service, IsolateRef isolateRef) async { - final isolateId = isolateRef.id!; - final isolate = await service.getIsolate(isolateId); - final stack = await service.getStack(isolateId); - - // Make sure we are in the right place. - expect(stack.frames!.length, greaterThanOrEqualTo(1)); - expect(stack.frames![0].function!.name, 'testFunction'); - - final Library root = - await service.getObject(isolateId, isolate.rootLib!.id!) as Library; - FuncRef funcRef = - root.functions!.singleWhere((f) => f.name == 'leafFunction'); - Func func = await service.getObject(isolateId, funcRef.id!) as Func; - - var expectedRange = { + coverageTest( + { 'scriptIndex': 0, 'startPos': 397, 'endPos': 447, @@ -94,21 +104,22 @@ var tests = [ 'hits': [397], 'misses': [] } - }; - - final location = func.location!; - final report = await service.getSourceReport( - isolateId, [SourceReportKind.kCoverage], - scriptId: location.script!.id, - tokenPos: location.tokenPos, - endTokenPos: location.endTokenPos, - forceCompile: true); - expect(report.ranges!.length, 1); - expect(report.ranges![0].toJson(), expectedRange); - expect(report.scripts!.length, 1); - expect( - report.scripts![0].uri, endsWith('coverage_leaf_function_test.dart')); - }, + }, + reportLines: false, + ), + coverageTest( + { + 'scriptIndex': 0, + 'startPos': 397, + 'endPos': 447, + 'compiled': true, + 'coverage': { + 'hits': [11], + 'misses': [] + } + }, + reportLines: true, + ), ]; main([args = const []]) => runIsolateTests( diff --git a/runtime/observatory/tests/service/get_version_rpc_test.dart b/runtime/observatory/tests/service/get_version_rpc_test.dart index 6bec5f157fd..340b04642a5 100644 --- a/runtime/observatory/tests/service/get_version_rpc_test.dart +++ b/runtime/observatory/tests/service/get_version_rpc_test.dart @@ -12,7 +12,7 @@ var tests = [ final result = await vm.invokeRpcNoUpgrade('getVersion', {}); expect(result['type'], 'Version'); expect(result['major'], 3); - expect(result['minor'], 50); + expect(result['minor'], 51); expect(result['_privateMajor'], 0); expect(result['_privateMinor'], 0); }, diff --git a/runtime/observatory_2/tests/service_2/get_version_rpc_test.dart b/runtime/observatory_2/tests/service_2/get_version_rpc_test.dart index 31490bf4ef5..9b7cdf502e1 100644 --- a/runtime/observatory_2/tests/service_2/get_version_rpc_test.dart +++ b/runtime/observatory_2/tests/service_2/get_version_rpc_test.dart @@ -12,7 +12,7 @@ var tests = [ final result = await vm.invokeRpcNoUpgrade('getVersion', {}); expect(result['type'], equals('Version')); expect(result['major'], equals(3)); - expect(result['minor'], equals(50)); + expect(result['minor'], equals(51)); expect(result['_privateMajor'], equals(0)); expect(result['_privateMinor'], equals(0)); }, diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc index 4985582031a..2b43622f228 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc @@ -3353,6 +3353,9 @@ static void GetSourceReport(Thread* thread, JSONStream* js) { compile_mode = SourceReport::kForceCompile; } + bool report_lines = + BoolParameter::Parse(js->LookupParam("reportLines"), false); + Script& script = Script::Handle(); intptr_t start_pos = UIntParameter::Parse(js->LookupParam("tokenPos")); intptr_t end_pos = UIntParameter::Parse(js->LookupParam("endTokenPos")); @@ -3383,7 +3386,7 @@ static void GetSourceReport(Thread* thread, JSONStream* js) { return; } } - SourceReport report(report_set, compile_mode); + SourceReport report(report_set, compile_mode, report_lines); report.PrintJSON(js, script, TokenPosition::Deserialize(start_pos), TokenPosition::Deserialize(end_pos)); #endif // !DART_PRECOMPILED_RUNTIME diff --git a/runtime/vm/service.h b/runtime/vm/service.h index 4c87d364639..38583b1c237 100644 --- a/runtime/vm/service.h +++ b/runtime/vm/service.h @@ -15,7 +15,7 @@ namespace dart { #define SERVICE_PROTOCOL_MAJOR_VERSION 3 -#define SERVICE_PROTOCOL_MINOR_VERSION 50 +#define SERVICE_PROTOCOL_MINOR_VERSION 51 class Array; class EmbedderServiceHandler; diff --git a/runtime/vm/service/service.md b/runtime/vm/service/service.md index 6e29e0f3d01..5e9b1dbcafc 100644 --- a/runtime/vm/service/service.md +++ b/runtime/vm/service/service.md @@ -1,8 +1,8 @@ -# Dart VM Service Protocol 3.49 +# Dart VM Service Protocol 3.51 > Please post feedback to the [observatory-discuss group][discuss-list] -This document describes of _version 3.49_ of the Dart VM Service Protocol. This +This document describes of _version 3.51_ of the Dart VM Service Protocol. This protocol is used to communicate with a running Dart Virtual Machine. To use the Service Protocol, start the VM with the *--observe* flag. @@ -1059,7 +1059,8 @@ SourceReport|Sentinel getSourceReport(string isolateId, string scriptId [optional], int tokenPos [optional], int endTokenPos [optional], - bool forceCompile [optional]) + bool forceCompile [optional], + bool reportLines [optional]) ``` The _getSourceReport_ RPC is used to generate a set of reports tied to @@ -1095,6 +1096,12 @@ cause a compilation error, which could terminate the running Dart program. If this parameter is not provided, it is considered to have the value _false_. +The _reportLines_ parameter changes the token positions in +_SourceReportRange.possibleBreakpoints_ and _SourceReportCoverage_ to be line +numbers. This is designed to reduce the number of RPCs that need to be performed +in the case that the client is only interested in line numbers. If this +parameter is not provided, it is considered to have the value _false_. + If _isolateId_ refers to an isolate which has exited, then the _Collected_ [Sentinel](#sentinel) is returned. @@ -3781,12 +3788,12 @@ locations in an isolate. ``` class SourceReportCoverage { - // A list of token positions in a SourceReportRange which have been - // executed. The list is sorted. + // A list of token positions (or line numbers if reportLines was enabled) in a + // SourceReportRange which have been executed. The list is sorted. int[] hits; - // A list of token positions in a SourceReportRange which have not been - // executed. The list is sorted. + // A list of token positions (or line numbers if reportLines was enabled) in a + // SourceReportRange which have not been executed. The list is sorted. int[] misses; } ``` @@ -3836,9 +3843,9 @@ class SourceReportRange { SourceReportCoverage coverage [optional]; // Possible breakpoint information for this range, represented as a - // sorted list of token positions. Provided only when the when the - // PossibleBreakpoint report has been requested and the range has been - // compiled. + // sorted list of token positions (or line numbers if reportLines was + // enabled). Provided only when the when the PossibleBreakpoint report has + // been requested and the range has been compiled. int[] possibleBreakpoints [optional]; } ``` @@ -4176,5 +4183,6 @@ version | comments 3.48 | Added `Profiler` stream, `UserTagChanged` event kind, and `updatedTag` and `previousTag` properties to `Event`. 3.49 | Added `CpuSamples` event kind, and `cpuSamples` property to `Event`. 3.50 | Added `returnType`, `parameters`, and `typeParameters` to `@Instance`, and `implicit` to `@Function`. Added `Parameter` type. +3.51 | Added optional `reportLines` parameter to `getSourceReport` RPC. [discuss-list]: https://groups.google.com/a/dartlang.org/forum/#!forum/observatory-discuss diff --git a/runtime/vm/source_report.cc b/runtime/vm/source_report.cc index 1bd8a934220..f72a72ea9c7 100644 --- a/runtime/vm/source_report.cc +++ b/runtime/vm/source_report.cc @@ -22,9 +22,12 @@ const char* SourceReport::kCoverageStr = "Coverage"; const char* SourceReport::kPossibleBreakpointsStr = "PossibleBreakpoints"; const char* SourceReport::kProfileStr = "_Profile"; -SourceReport::SourceReport(intptr_t report_set, CompileMode compile_mode) +SourceReport::SourceReport(intptr_t report_set, + CompileMode compile_mode, + bool report_lines) : report_set_(report_set), compile_mode_(compile_mode), + report_lines_(report_lines), thread_(NULL), script_(NULL), start_pos_(TokenPosition::kMinSource), @@ -218,6 +221,17 @@ void SourceReport::PrintCallSitesData(JSONObject* jsobj, } } +intptr_t SourceReport::GetTokenPosOrLine(const Script& script, + const TokenPosition& token_pos) { + if (!report_lines_) { + return token_pos.Pos(); + } + intptr_t line = -1; + const bool found = script.GetTokenLocation(token_pos, &line); + ASSERT(found); + return line; +} + void SourceReport::PrintCoverageData(JSONObject* jsobj, const Function& function, const Code& code) { @@ -230,6 +244,7 @@ void SourceReport::PrintCoverageData(JSONObject* jsobj, function.RestoreICDataMap(ic_data_array, false /* clone ic-data */); const PcDescriptors& descriptors = PcDescriptors::Handle(zone(), code.pc_descriptors()); + const Script& script = Script::Handle(zone(), function.script()); const int kCoverageNone = 0; const int kCoverageMiss = 1; @@ -276,20 +291,24 @@ void SourceReport::PrintCoverageData(JSONObject* jsobj, JSONObject cov(jsobj, "coverage"); { JSONArray hits(&cov, "hits"); + TokenPosition pos = begin_pos; for (int i = 0; i < func_length; i++) { if (coverage[i] == kCoverageHit) { - // Add the token position of the hit. - hits.AddValue(begin_pos.Pos() + i); + // Add the token position or line number of the hit. + hits.AddValue(GetTokenPosOrLine(script, pos)); } + pos = pos.Next(); } } { JSONArray misses(&cov, "misses"); + TokenPosition pos = begin_pos; for (int i = 0; i < func_length; i++) { if (coverage[i] == kCoverageMiss) { - // Add the token position of the miss. - misses.AddValue(begin_pos.Pos() + i); + // Add the token position or line number of the miss. + misses.AddValue(GetTokenPosOrLine(script, pos)); } + pos = pos.Next(); } } } @@ -311,6 +330,7 @@ void SourceReport::PrintPossibleBreakpointsData(JSONObject* jsobj, const PcDescriptors& descriptors = PcDescriptors::Handle(zone(), code.pc_descriptors()); + const Script& script = Script::Handle(zone(), func.script()); PcDescriptors::Iterator iter(descriptors, kSafepointKind); while (iter.MoveNext()) { @@ -324,11 +344,13 @@ void SourceReport::PrintPossibleBreakpointsData(JSONObject* jsobj, } JSONArray bpts(jsobj, "possibleBreakpoints"); + TokenPosition pos = begin_pos; for (int i = 0; i < func_length; i++) { if (possible.Contains(i)) { - // Add the token position. - bpts.AddValue(begin_pos.Pos() + i); + // Add the token position or line number. + bpts.AddValue(GetTokenPosOrLine(script, pos)); } + pos = pos.Next(); } } @@ -652,7 +674,7 @@ void SourceReport::CollectConstConstructorCoverageFromScripts( JSONObject cov(&range, "coverage"); { JSONArray hits(&cov, "hits"); - hits.AddValue(begin_pos); + hits.AddValue(GetTokenPosOrLine(scriptRef, begin_pos)); } { JSONArray misses(&cov, "misses"); diff --git a/runtime/vm/source_report.h b/runtime/vm/source_report.h index a7acf0041a0..070ca0a98e2 100644 --- a/runtime/vm/source_report.h +++ b/runtime/vm/source_report.h @@ -38,7 +38,9 @@ class SourceReport { // report_set is a bitvector indicating which reports to generate // (e.g. kCallSites | kCoverage). - explicit SourceReport(intptr_t report_set, CompileMode compile = kNoCompile); + explicit SourceReport(intptr_t report_set, + CompileMode compile = kNoCompile, + bool report_lines = false); ~SourceReport(); // Generate a source report for (some subrange of) a script. @@ -66,6 +68,8 @@ class SourceReport { bool ShouldSkipField(const Field& field); intptr_t GetScriptIndex(const Script& script); bool ScriptIsLoadedByLibrary(const Script& script, const Library& lib); + intptr_t GetTokenPosOrLine(const Script& script, + const TokenPosition& token_pos); void PrintCallSitesData(JSONObject* jsobj, const Function& func, @@ -126,6 +130,7 @@ class SourceReport { intptr_t report_set_; CompileMode compile_mode_; + bool report_lines_; Thread* thread_; const Script* script_; TokenPosition start_pos_;