Skip to content

Commit

Permalink
Fix benchmark reload bug and migrate away from deprecated js_util A…
Browse files Browse the repository at this point in the history
…PIs (#5577)

This PR:
- Ensures the benchmark client reloads with the proper `initialPage`. Without this change, the benchmark app may not reload properly, which causes a hang on the server.
- Migrates away from `js_util` APIs to `dart:ui_web`. This was causing spurious deprecation warnings.

The changes in this PR are test covered by existing tests.
  • Loading branch information
kenzieschmoll authored Dec 6, 2023
1 parent afd4517 commit 9a55d4c
Show file tree
Hide file tree
Showing 8 changed files with 44 additions and 23 deletions.
5 changes: 5 additions & 0 deletions packages/web_benchmarks/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.1.0+10

* Ensure the benchmark client reloads with the proper `initialPage`.
* Migrates benchmark recorder away from deprecated `js_util` APIs.

## 0.1.0+9

* Updates minimum supported SDK version to Flutter 3.10/Dart 3.0.
Expand Down
19 changes: 17 additions & 2 deletions packages/web_benchmarks/lib/client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ final LocalBenchmarkServerClient _client = LocalBenchmarkServerClient();
///
/// When used without a server, prompts the user to select a benchmark to
/// run next.
Future<void> runBenchmarks(Map<String, RecorderFactory> benchmarks) async {
Future<void> runBenchmarks(
Map<String, RecorderFactory> benchmarks, {
String initialPage = defaultInitialPage,
}) async {
// Set local benchmarks.
_benchmarks = benchmarks;

Expand All @@ -43,7 +46,19 @@ Future<void> runBenchmarks(Map<String, RecorderFactory> benchmarks) async {
}

await _runBenchmark(nextBenchmark);
html.window.location.reload();

final Uri currentUri = Uri.parse(html.window.location.href);
// Create a new URI with the current 'page' value set to [initialPage] to
// ensure the benchmark app is reloaded at the proper location.
final Uri newUri = Uri(
scheme: currentUri.scheme,
host: currentUri.host,
port: currentUri.port,
path: initialPage,
);

// Reloading the window will trigger the next benchmark to run.
html.window.location.replace(newUri.toString());
}

Future<void> _runBenchmark(String? benchmarkName) async {
Expand Down
3 changes: 2 additions & 1 deletion packages/web_benchmarks/lib/server.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'dart:io' as io;
import 'package:logging/logging.dart';

import 'src/benchmark_result.dart';
import 'src/common.dart';
import 'src/runner.dart';

export 'src/benchmark_result.dart';
Expand Down Expand Up @@ -47,7 +48,7 @@ Future<BenchmarkResults> serveWebBenchmark({
int chromeDebugPort = defaultChromeDebugPort,
bool headless = true,
bool treeShakeIcons = true,
String initialPage = BenchmarkServer.defaultInitialPage,
String initialPage = defaultInitialPage,
}) async {
// Reduce logging level. Otherwise, package:webkit_inspection_protocol is way too spammy.
Logger.root.level = Level.INFO;
Expand Down
4 changes: 4 additions & 0 deletions packages/web_benchmarks/lib/src/common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,7 @@ const int kMeasuredSampleCount = 100;
/// A special value returned by the `/next-benchmark` HTTP POST request when
/// all benchmarks have run and there are no more benchmarks to run.
const String kEndOfBenchmarks = '__end_of_benchmarks__';

/// The default initial page to load upon opening the benchmark app or reloading
/// it in Chrome.
const String defaultInitialPage = 'index.html';
21 changes: 11 additions & 10 deletions packages/web_benchmarks/lib/src/recorder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@

import 'dart:async';
import 'dart:html' as html;
import 'dart:js';
import 'dart:js_util' as js_util;
import 'dart:math' as math;
import 'dart:ui';
import 'dart:ui_web' as ui_web;

import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart';
Expand Down Expand Up @@ -1211,19 +1210,21 @@ final Map<String, EngineBenchmarkValueListener> _engineBenchmarkListeners =
///
/// If another listener is already registered, overrides it.
void registerEngineBenchmarkValueListener(
String name, EngineBenchmarkValueListener listener) {
String name,
EngineBenchmarkValueListener listener,
) {
if (_engineBenchmarkListeners.containsKey(name)) {
throw StateError('A listener for "$name" is already registered.\n'
'Call `stopListeningToEngineBenchmarkValues` to unregister the previous '
'listener before registering a new one.');
throw StateError(
'A listener for "$name" is already registered.\n'
'Call `stopListeningToEngineBenchmarkValues` to unregister the previous '
'listener before registering a new one.',
);
}

if (_engineBenchmarkListeners.isEmpty) {
// The first listener is being registered. Register the global listener.
js_util.setProperty(html.window, '_flutter_internal_on_benchmark',
allowInterop(_dispatchEngineBenchmarkValue));
ui_web.benchmarkValueCallback = _dispatchEngineBenchmarkValue;
}

_engineBenchmarkListeners[name] = listener;
}

Expand All @@ -1232,7 +1233,7 @@ void stopListeningToEngineBenchmarkValues(String name) {
_engineBenchmarkListeners.remove(name);
if (_engineBenchmarkListeners.isEmpty) {
// The last listener unregistered. Remove the global listener.
js_util.setProperty(html.window, '_flutter_internal_on_benchmark', null);
ui_web.benchmarkValueCallback = null;
}
}

Expand Down
5 changes: 0 additions & 5 deletions packages/web_benchmarks/lib/src/runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,6 @@ class BenchmarkServer {
this.initialPage = defaultInitialPage,
});

/// The default initial page to load upon opening the benchmark app in Chrome.
///
/// This value will be used by default if no value is passed to [initialPage].
static const String defaultInitialPage = 'index.html';

final ProcessManager _processManager = const LocalProcessManager();

/// The directory containing the app that's being benchmarked.
Expand Down
6 changes: 3 additions & 3 deletions packages/web_benchmarks/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ name: web_benchmarks
description: A benchmark harness for performance-testing Flutter apps in Chrome.
repository: https://github.com/flutter/packages/tree/main/packages/web_benchmarks
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+web_benchmarks%22
version: 0.1.0+9
version: 0.1.0+10

environment:
sdk: ">=3.0.0 <4.0.0"
flutter: ">=3.10.0"
sdk: ">=3.2.0 <4.0.0"
flutter: ">=3.16.0"

dependencies:
flutter:
Expand Down
4 changes: 2 additions & 2 deletions packages/web_benchmarks/testing/web_benchmarks_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import 'dart:io';
import 'package:test/test.dart';

import 'package:web_benchmarks/server.dart';
import 'package:web_benchmarks/src/runner.dart';
import 'package:web_benchmarks/src/common.dart';

Future<void> main() async {
test('Can run a web benchmark', () async {
Expand All @@ -30,7 +30,7 @@ Future<void> main() async {
Future<void> _runBenchmarks({
required List<String> benchmarkNames,
required String entryPoint,
String initialPage = BenchmarkServer.defaultInitialPage,
String initialPage = defaultInitialPage,
}) async {
final BenchmarkResults taskResult = await serveWebBenchmark(
benchmarkAppDirectory: Directory('testing/test_app'),
Expand Down

0 comments on commit 9a55d4c

Please sign in to comment.