From 06a27e1b4a9a357d84bd6e9c95c046607c20f3a3 Mon Sep 17 00:00:00 2001 From: Kenzie Davisson <43759233+kenzieschmoll@users.noreply.github.com> Date: Wed, 9 Oct 2024 09:23:07 -0700 Subject: [PATCH] Unskip the wasm benchmark (#8320) --- .../benchmark/devtools_benchmarks_test.dart | 57 +++++++++---------- .../scripts/dart2wasm_performance_diff.dart | 6 +- .../benchmark/scripts/run_benchmarks.dart | 4 +- .../automators/devtools_automator.dart | 12 ++++ .../test_infra/client/client_js.dart | 17 ++++++ .../client_shared.dart} | 15 +---- .../test_infra/client/client_wasm.dart | 17 ++++++ .../benchmark/test_infra/common.dart | 21 +++++-- .../test_infra/devtools_recorder.dart | 1 + packages/devtools_app/pubspec.yaml | 2 +- 10 files changed, 100 insertions(+), 52 deletions(-) create mode 100644 packages/devtools_app/benchmark/test_infra/client/client_js.dart rename packages/devtools_app/benchmark/test_infra/{client.dart => client/client_shared.dart} (57%) create mode 100644 packages/devtools_app/benchmark/test_infra/client/client_wasm.dart diff --git a/packages/devtools_app/benchmark/devtools_benchmarks_test.dart b/packages/devtools_app/benchmark/devtools_benchmarks_test.dart index a6c64226d6f..1f901430235 100644 --- a/packages/devtools_app/benchmark/devtools_benchmarks_test.dart +++ b/packages/devtools_app/benchmark/devtools_benchmarks_test.dart @@ -8,27 +8,19 @@ import 'dart:convert' show JsonEncoder; import 'dart:io'; +import 'package:collection/collection.dart'; import 'package:test/test.dart'; +import 'package:web_benchmarks/metrics.dart'; import 'package:web_benchmarks/server.dart'; import 'test_infra/common.dart'; import 'test_infra/project_root_directory.dart'; -List metricList({required bool useWasm}) => [ - // The skwasm renderer doesn't have preroll or apply frame steps in its - // rendering. - if (!useWasm) ...[ - 'preroll_frame', - 'apply_frame', - ], - 'drawFrameDuration', - ]; +const _isWasmScore = 'isWasm'; -final valueList = [ - 'average', - 'outlierAverage', - 'outlierRatio', - 'noise', +const _extraScores = [ + totalUiFrameAverage, + _isWasmScore, ]; /// Tests that the DevTools web benchmarks are run and reported correctly. @@ -40,10 +32,6 @@ void main() { await _runBenchmarks(useWasm: useWasm); }, timeout: const Timeout(Duration(minutes: 10)), - // TODO(https://github.com/dart-lang/sdk/issues/56664): unskip the wasm - // benchmarks once this issue is resolved and we can bump DevTools to a - // version of Flutter that includes the fix. - skip: useWasm, ); } @@ -54,12 +42,12 @@ Future _runBenchmarks({bool useWasm = false}) async { stdout.writeln('Starting web benchmark tests ...'); final taskResult = await serveWebBenchmark( benchmarkAppDirectory: projectRootDirectory(), - entryPoint: 'benchmark/test_infra/client.dart', + entryPoint: generateBenchmarkEntryPoint(useWasm: useWasm), compilationOptions: useWasm ? const CompilationOptions.wasm() : const CompilationOptions.js(), treeShakeIcons: false, - initialPage: benchmarkInitialPage, + benchmarkPath: benchmarkPath(useWasm: useWasm), ); stdout.writeln('Web benchmark tests finished.'); @@ -69,17 +57,24 @@ Future _runBenchmarks({bool useWasm = false}) async { ); for (final benchmarkName in DevToolsBenchmark.values.map((e) => e.id)) { - final expectedMetrics = metricList(useWasm: useWasm); + final expectedMetrics = expectedBenchmarkMetrics(useWasm: useWasm) + .map((BenchmarkMetric metric) => metric.label) + .toList(); + const expectedComputations = BenchmarkMetricComputation.values; + final scores = taskResult.scores[benchmarkName] ?? []; expect( - taskResult.scores[benchmarkName], - hasLength(expectedMetrics.length * valueList.length + 1), + scores, + hasLength( + expectedMetrics.length * expectedComputations.length + + _extraScores.length, + ), ); for (final metricName in expectedMetrics) { - for (final valueName in valueList) { + for (final computation in expectedComputations) { expect( - taskResult.scores[benchmarkName]?.where( - (score) => score.metric == '$metricName.$valueName', + scores.where( + (score) => score.metric == '$metricName.${computation.name}', ), hasLength(1), ); @@ -87,11 +82,15 @@ Future _runBenchmarks({bool useWasm = false}) async { } expect( - taskResult.scores[benchmarkName]?.where( - (score) => score.metric == 'totalUiFrame.average', - ), + scores.where((score) => score.metric == totalUiFrameAverage), hasLength(1), ); + + final isWasmScore = scores.firstWhereOrNull( + (BenchmarkScore score) => score.metric == _isWasmScore, + ); + expect(isWasmScore, isNotNull); + expect(isWasmScore!.value, useWasm ? 1 : 0); } expect( diff --git a/packages/devtools_app/benchmark/scripts/dart2wasm_performance_diff.dart b/packages/devtools_app/benchmark/scripts/dart2wasm_performance_diff.dart index da2075110af..3aa2f3cf727 100644 --- a/packages/devtools_app/benchmark/scripts/dart2wasm_performance_diff.dart +++ b/packages/devtools_app/benchmark/scripts/dart2wasm_performance_diff.dart @@ -42,13 +42,13 @@ import 'utils.dart'; /// using the '--baseline' or '--test' arguments. /// /// Example usage: -/// * dart run benchmark/scripts/run_benchmarks.dart +/// * dart run benchmark/scripts/dart2wasm_performance_diff.dart /// /// Example usage that averages benchmark results over 5 runs: -/// * dart run benchmark/scripts/run_benchmarks.dart --average-of=5 +/// * dart run benchmark/scripts/dart2wasm_performance_diff.dart --average-of=5 /// /// Example usage that diffs against an existing basline: -/// * dart run benchmark/scripts/run_benchmarks.dart --baseline=/Users/me/Downloads/baseline_run.json +/// * dart run benchmark/scripts/dart2wasm_performance_diff.dart --baseline=/Users/me/Downloads/baseline_run.json void main(List args) async { if (!Directory.current.path.contains('devtools_app')) { stderr.writeln( diff --git a/packages/devtools_app/benchmark/scripts/run_benchmarks.dart b/packages/devtools_app/benchmark/scripts/run_benchmarks.dart index c130b187755..e2c463f4934 100644 --- a/packages/devtools_app/benchmark/scripts/run_benchmarks.dart +++ b/packages/devtools_app/benchmark/scripts/run_benchmarks.dart @@ -55,12 +55,12 @@ Future runBenchmarks({ benchmarkResults.add( await serveWebBenchmark( benchmarkAppDirectory: projectRootDirectory(), - entryPoint: 'benchmark/test_infra/client.dart', + entryPoint: generateBenchmarkEntryPoint(useWasm: useWasm), compilationOptions: useWasm ? const CompilationOptions.wasm() : const CompilationOptions.js(), treeShakeIcons: false, - initialPage: benchmarkInitialPage, + benchmarkPath: benchmarkPath(useWasm: useWasm), headless: !useBrowser, ), ); diff --git a/packages/devtools_app/benchmark/test_infra/automators/devtools_automator.dart b/packages/devtools_app/benchmark/test_infra/automators/devtools_automator.dart index 48613274d1b..034f380f809 100644 --- a/packages/devtools_app/benchmark/test_infra/automators/devtools_automator.dart +++ b/packages/devtools_app/benchmark/test_infra/automators/devtools_automator.dart @@ -9,8 +9,10 @@ import 'dart:async'; import 'package:devtools_app/devtools_app.dart'; import 'package:devtools_test/helpers.dart'; import 'package:devtools_test/test_data.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:web_benchmarks/client.dart'; import '../common.dart'; import '_cpu_profiler_automator.dart'; @@ -21,6 +23,7 @@ class DevToolsAutomater { DevToolsAutomater({ required this.benchmark, required this.stopWarmingUpCallback, + required this.profile, }); /// The current benchmark. @@ -32,6 +35,9 @@ class DevToolsAutomater { /// as over. final void Function() stopWarmingUpCallback; + /// The profile collected for the running benchmark + final Profile profile; + /// Whether the automation has ended. bool finished = false; @@ -65,6 +71,12 @@ class DevToolsAutomater { await PerformanceScreenAutomator(controller).run(); } + // Record whether we are in wasm mode or not. Ideally, we'd have a more + // first-class way to add metadata like this, but this will work for us to + // pass information about the environment back to the server for the + // purposes of our own tests. + profile.extraData['isWasm'] = kIsWasm ? 1 : 0; + // At the end of the test, mark as finished. finished = true; } diff --git a/packages/devtools_app/benchmark/test_infra/client/client_js.dart b/packages/devtools_app/benchmark/test_infra/client/client_js.dart new file mode 100644 index 00000000000..92ccf72d7ba --- /dev/null +++ b/packages/devtools_app/benchmark/test_infra/client/client_js.dart @@ -0,0 +1,17 @@ +// Copyright 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:web_benchmarks/client.dart'; + +import '../common.dart'; +import 'client_shared.dart'; + +/// Runs the client of the DevTools web benchmarks. +/// +/// When the DevTools web benchmarks are run, the server builds an app with this +/// file as the entry point (see `run_benchmarks.dart`). The app automates +/// the DevTools web app, records some performance data, and reports them. +Future main() async { + await runBenchmarks(benchmarks, benchmarkPath: benchmarkPath(useWasm: false)); +} diff --git a/packages/devtools_app/benchmark/test_infra/client.dart b/packages/devtools_app/benchmark/test_infra/client/client_shared.dart similarity index 57% rename from packages/devtools_app/benchmark/test_infra/client.dart rename to packages/devtools_app/benchmark/test_infra/client/client_shared.dart index cc59063bfd2..66e86a76d0f 100644 --- a/packages/devtools_app/benchmark/test_infra/client.dart +++ b/packages/devtools_app/benchmark/test_infra/client/client_shared.dart @@ -1,11 +1,11 @@ -// Copyright 2023 The Chromium Authors. All rights reserved. +// Copyright 2024 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. import 'package:web_benchmarks/client.dart'; -import 'common.dart'; -import 'devtools_recorder.dart'; +import '../common.dart'; +import '../devtools_recorder.dart'; typedef RecorderFactory = Recorder Function(); @@ -20,12 +20,3 @@ final benchmarks = { benchmark: DevToolsBenchmark.offlinePerformanceScreen, ), }; - -/// Runs the client of the DevTools web benchmarks. -/// -/// When the DevTools web benchmarks are run, the server builds an app with this -/// file as the entry point (see `run_benchmarks.dart`). The app automates -/// the DevTools web app, records some performance data, and reports them. -Future main() async { - await runBenchmarks(benchmarks, initialPage: benchmarkInitialPage); -} diff --git a/packages/devtools_app/benchmark/test_infra/client/client_wasm.dart b/packages/devtools_app/benchmark/test_infra/client/client_wasm.dart new file mode 100644 index 00000000000..bc133d33c45 --- /dev/null +++ b/packages/devtools_app/benchmark/test_infra/client/client_wasm.dart @@ -0,0 +1,17 @@ +// Copyright 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:web_benchmarks/client.dart'; + +import '../common.dart'; +import 'client_shared.dart'; + +/// Runs the client of the DevTools web benchmarks. +/// +/// When the DevTools web benchmarks are run, the server builds an app with this +/// file as the entry point (see `run_benchmarks.dart`). The app automates +/// the DevTools web app, records some performance data, and reports them. +Future main() async { + await runBenchmarks(benchmarks, benchmarkPath: benchmarkPath(useWasm: true)); +} diff --git a/packages/devtools_app/benchmark/test_infra/common.dart b/packages/devtools_app/benchmark/test_infra/common.dart index c3f5b462d52..939d881efaa 100644 --- a/packages/devtools_app/benchmark/test_infra/common.dart +++ b/packages/devtools_app/benchmark/test_infra/common.dart @@ -4,11 +4,22 @@ /// The initial page to load upon opening the DevTools benchmark app or /// reloading it in Chrome. -// -// We use an empty initial page so that the benchmark server does not attempt -// to load the default page 'index.html', which will show up as "page not -// found" in DevTools. -const benchmarkInitialPage = ''; +/// +/// We use an empty initial page so that the benchmark server does not attempt +/// to load the default page 'index.html', which will show up as "page not +/// found" in DevTools. +const _benchmarkInitialPage = ''; + +const _wasmQueryParameters = {'wasm': 'true'}; + +String benchmarkPath({required bool useWasm}) => Uri( + path: _benchmarkInitialPage, + queryParameters: useWasm ? _wasmQueryParameters : null, + ).toString(); + +String generateBenchmarkEntryPoint({required bool useWasm}) { + return 'benchmark/test_infra/client/client_${useWasm ? 'wasm' : 'js'}.dart'; +} const devtoolsBenchmarkPrefix = 'devtools'; diff --git a/packages/devtools_app/benchmark/test_infra/devtools_recorder.dart b/packages/devtools_app/benchmark/test_infra/devtools_recorder.dart index 2353fd7b75c..7a5d178ff8b 100644 --- a/packages/devtools_app/benchmark/test_infra/devtools_recorder.dart +++ b/packages/devtools_app/benchmark/test_infra/devtools_recorder.dart @@ -34,6 +34,7 @@ class DevToolsRecorder extends WidgetRecorder { _devToolsAutomator = DevToolsAutomater( benchmark: benchmark, stopWarmingUpCallback: profile.stopWarmingUp, + profile: profile, ); return _devToolsAutomator!.createWidget(); } diff --git a/packages/devtools_app/pubspec.yaml b/packages/devtools_app/pubspec.yaml index fae75a3eeaf..118354f63c1 100644 --- a/packages/devtools_app/pubspec.yaml +++ b/packages/devtools_app/pubspec.yaml @@ -76,7 +76,7 @@ dev_dependencies: mockito: ^5.4.1 stager: ^1.0.1 test: ^1.21.0 - web_benchmarks: ^2.0.0 + web_benchmarks: ^3.1.0 webkit_inspection_protocol: ">=0.5.0 <2.0.0" flutter: