Skip to content

Commit

Permalink
Update VelocityTracker (4) (flutter#139166)
Browse files Browse the repository at this point in the history 
This updates the implementation to use the stopwatch from the Clock object and pipes it through to the TestWidgetsFlutterBinding so it will be kept in sync with FakeAsync.

Relands flutter#138843 attempted to reland flutter#137381 which attempted to reland flutter#132291
Fixes flutter#97761

1. The original change was reverted due to flakiness it introduced in tests that use fling gestures.
  * Using a mocked clock through the test binding fixes this now
2. It was reverted a second time because a change at tip of tree broke it, exposing memory leaks, but it was not rebased before landing. 
  * These leaks are now fixed
3. It was reverted a third time, because we were so excellently quick to revert those other times, that we did not notice the broken benchmark that only runs in postsubmit.
  * The benchmark is now fixed
  • Loading branch information
@Piinks @caseycrogers
Piinks authored and caseycrogers committed Dec 29, 2023
1 parent 324421a commit ea3f38d
Show file tree
Hide file tree
Showing 9 changed files with 178 additions and 65 deletions.
52 changes: 29 additions & 23 deletions dev/benchmarks/microbenchmarks/lib/gestures/velocity_tracker_bench.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

import 'package:flutter/gestures.dart';
import 'package:flutter_test/flutter_test.dart';

import '../common.dart';
import 'data/velocity_tracker_data.dart';
Expand All @@ -16,38 +17,43 @@ class TrackerBenchmark {
final String name;
}

void main() {
Future<void> main() async {
assert(false, "Don't run benchmarks in debug mode! Use 'flutter run --release'.");
final BenchmarkResultPrinter printer = BenchmarkResultPrinter();
final List<TrackerBenchmark> benchmarks = <TrackerBenchmark>[
TrackerBenchmark(name: 'velocity_tracker_iteration', tracker: VelocityTracker.withKind(PointerDeviceKind.touch)),
TrackerBenchmark(name: 'velocity_tracker_iteration_ios_fling', tracker: IOSScrollViewFlingVelocityTracker(PointerDeviceKind.touch)),
TrackerBenchmark(name: 'velocity_tracker_iteration',
tracker: VelocityTracker.withKind(PointerDeviceKind.touch)),
TrackerBenchmark(name: 'velocity_tracker_iteration_ios_fling',
tracker: IOSScrollViewFlingVelocityTracker(PointerDeviceKind.touch)),
];
final Stopwatch watch = Stopwatch();

for (final TrackerBenchmark benchmark in benchmarks) {
print('${benchmark.name} benchmark...');
final VelocityTracker tracker = benchmark.tracker;
watch.reset();
watch.start();
for (int i = 0; i < _kNumIters; i += 1) {
for (final PointerEvent event in velocityEventData) {
if (event is PointerDownEvent || event is PointerMoveEvent) {
tracker.addPosition(event.timeStamp, event.position);
}
if (event is PointerUpEvent) {
tracker.getVelocity();
await benchmarkWidgets((WidgetTester tester) async {
for (final TrackerBenchmark benchmark in benchmarks) {
print('${benchmark.name} benchmark...');
final VelocityTracker tracker = benchmark.tracker;
watch.reset();
watch.start();
for (int i = 0; i < _kNumIters; i += 1) {
for (final PointerEvent event in velocityEventData) {
if (event is PointerDownEvent || event is PointerMoveEvent) {
tracker.addPosition(event.timeStamp, event.position);
}
if (event is PointerUpEvent) {
tracker.getVelocity();
}
}
}
watch.stop();

printer.addResult(
description: 'Velocity tracker: ${tracker.runtimeType}',
value: watch.elapsedMicroseconds / _kNumIters,
unit: 'µs per iteration',
name: benchmark.name,
);
}
watch.stop();
printer.addResult(
description: 'Velocity tracker: ${tracker.runtimeType}',
value: watch.elapsedMicroseconds / _kNumIters,
unit: 'µs per iteration',
name: benchmark.name,
);
}
});

printer.printToStdout();
}
23 changes: 14 additions & 9 deletions packages/flutter/lib/src/gestures/binding.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ mixin GestureBinding on BindingBase implements HitTestable, HitTestDispatcher, H

if (resamplingEnabled) {
_resampler.addOrDispatch(event);
_resampler.sample(samplingOffset, _samplingClock);
_resampler.sample(samplingOffset, samplingClock);
return;
}

Expand Down Expand Up @@ -512,24 +512,29 @@ mixin GestureBinding on BindingBase implements HitTestable, HitTestDispatcher, H
_hitTests.clear();
}

/// Overrides the sampling clock for debugging and testing.
///
/// This value is ignored in non-debug builds.
@protected
SamplingClock? get debugSamplingClock => null;

void _handleSampleTimeChanged() {
if (!locked) {
if (resamplingEnabled) {
_resampler.sample(samplingOffset, _samplingClock);
_resampler.sample(samplingOffset, samplingClock);
}
else {
_resampler.stop();
}
}
}

SamplingClock get _samplingClock {
/// Overrides the sampling clock for debugging and testing.
///
/// This value is ignored in non-debug builds.
@protected
SamplingClock? get debugSamplingClock => null;

/// Provides access to the current [DateTime] and `StopWatch` objects for
/// sampling.
///
/// Overridden by [debugSamplingClock] for debug builds and testing. Using
/// this object under test will maintain synchronization with [FakeAsync].
SamplingClock get samplingClock {
SamplingClock value = SamplingClock();
assert(() {
final SamplingClock? debugValue = debugSamplingClock;
Expand Down
42 changes: 42 additions & 0 deletions packages/flutter/lib/src/gestures/velocity_tracker.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import 'package:flutter/foundation.dart';

import 'binding.dart';
import 'events.dart';
import 'lsq_solver.dart';

Expand Down Expand Up @@ -149,12 +150,21 @@ class VelocityTracker {
/// The kind of pointer this tracker is for.
final PointerDeviceKind kind;

// Time difference since the last sample was added
Stopwatch get _sinceLastSample {
_stopwatch ??= GestureBinding.instance.samplingClock.stopwatch();
return _stopwatch!;
}
Stopwatch? _stopwatch;

// Circular buffer; current sample at _index.
final List<_PointAtTime?> _samples = List<_PointAtTime?>.filled(_historySize, null);
int _index = 0;

/// Adds a position as the given time to the tracker.
void addPosition(Duration time, Offset position) {
_sinceLastSample.start();
_sinceLastSample.reset();
_index += 1;
if (_index == _historySize) {
_index = 0;
Expand All @@ -169,6 +179,16 @@ class VelocityTracker {
///
/// Returns null if there is no data on which to base an estimate.
VelocityEstimate? getVelocityEstimate() {
// Has user recently moved since last sample?
if (_sinceLastSample.elapsedMilliseconds > _assumePointerMoveStoppedMilliseconds) {
return const VelocityEstimate(
pixelsPerSecond: Offset.zero,
confidence: 1.0,
duration: Duration.zero,
offset: Offset.zero,
);
}

final List<double> x = <double>[];
final List<double> y = <double>[];
final List<double> w = <double>[];
Expand Down Expand Up @@ -288,6 +308,8 @@ class IOSScrollViewFlingVelocityTracker extends VelocityTracker {

@override
void addPosition(Duration time, Offset position) {
_sinceLastSample.start();
_sinceLastSample.reset();
assert(() {
final _PointAtTime? previousPoint = _touchSamples[_index];
if (previousPoint == null || previousPoint.time <= time) {
Expand Down Expand Up @@ -326,6 +348,16 @@ class IOSScrollViewFlingVelocityTracker extends VelocityTracker {

@override
VelocityEstimate getVelocityEstimate() {
// Has user recently moved since last sample?
if (_sinceLastSample.elapsedMilliseconds > VelocityTracker._assumePointerMoveStoppedMilliseconds) {
return const VelocityEstimate(
pixelsPerSecond: Offset.zero,
confidence: 1.0,
duration: Duration.zero,
offset: Offset.zero,
);
}

// The velocity estimated using this expression is an approximation of the
// scroll velocity of an iOS scroll view at the moment the user touch was
// released, not the final velocity of the iOS pan gesture recognizer
Expand Down Expand Up @@ -387,6 +419,16 @@ class MacOSScrollViewFlingVelocityTracker extends IOSScrollViewFlingVelocityTrac

@override
VelocityEstimate getVelocityEstimate() {
// Has user recently moved since last sample?
if (_sinceLastSample.elapsedMilliseconds > VelocityTracker._assumePointerMoveStoppedMilliseconds) {
return const VelocityEstimate(
pixelsPerSecond: Offset.zero,
confidence: 1.0,
duration: Duration.zero,
offset: Offset.zero,
);
}

// The velocity estimated using this expression is an approximation of the
// scroll velocity of a macOS scroll view at the moment the user touch was
// released.
Expand Down
10 changes: 10 additions & 0 deletions packages/flutter/test/gestures/double_tap_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ void main() {

setUp(() {
tap = DoubleTapGestureRecognizer();
addTearDown(tap.dispose);

doubleTapRecognized = false;
tap.onDoubleTap = () {
Expand Down Expand Up @@ -156,6 +157,7 @@ void main() {
final DoubleTapGestureRecognizer tapSecondary = DoubleTapGestureRecognizer(
allowedButtonsFilter: (int buttons) => buttons == kSecondaryButton,
);
addTearDown(tapSecondary.dispose);
tapSecondary.onDoubleTap = () {
doubleTapRecognized = true;
};
Expand Down Expand Up @@ -545,6 +547,7 @@ void main() {
final DoubleTapGestureRecognizer tapPrimary = DoubleTapGestureRecognizer(
allowedButtonsFilter: (int buttons) => buttons == kPrimaryButton,
);
addTearDown(tapPrimary.dispose);
tapPrimary.onDoubleTap = () {
doubleTapRecognized = true;
};
Expand Down Expand Up @@ -647,14 +650,17 @@ void main() {
..onTapDown = (TapDownDetails details) {
recognized.add('tapPrimary');
};
addTearDown(tapPrimary.dispose);
tapSecondary = TapGestureRecognizer()
..onSecondaryTapDown = (TapDownDetails details) {
recognized.add('tapSecondary');
};
addTearDown(tapSecondary.dispose);
doubleTap = DoubleTapGestureRecognizer()
..onDoubleTap = () {
recognized.add('doubleTap');
};
addTearDown(doubleTap.dispose);
});

tearDown(() {
Expand Down Expand Up @@ -692,6 +698,7 @@ void main() {
..onDoubleTap = () {
recognized.add('primary');
};
addTearDown(doubleTap.dispose);

// Down/up pair 7: normal tap sequence close to pair 6
const PointerDownEvent down7 = PointerDownEvent(
Expand Down Expand Up @@ -730,6 +737,7 @@ void main() {
..onDoubleTap = () {
recognized.add('primary');
};
addTearDown(doubleTap.dispose);

// Down/up pair 7: normal tap sequence close to pair 6
const PointerDownEvent down7 = PointerDownEvent(
Expand Down Expand Up @@ -765,8 +773,10 @@ void main() {
int tapCount = 0;
final DoubleTapGestureRecognizer doubleTap = DoubleTapGestureRecognizer()
..onDoubleTap = () {};
addTearDown(doubleTap.dispose);
final TapGestureRecognizer tap = TapGestureRecognizer()
..onTap = () => tapCount++;
addTearDown(tap.dispose);

// Open a arena with 2 members and holding.
doubleTap.addPointer(down1);
Expand Down
24 changes: 3 additions & 21 deletions packages/flutter/test/gestures/gesture_binding_resample_event_on_widget_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,17 @@

import 'dart:ui' as ui;

import 'package:clock/clock.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

class TestResampleEventFlutterBinding extends AutomatedTestWidgetsFlutterBinding {
@override
SamplingClock? get debugSamplingClock => TestSamplingClock(this.clock);
}

class TestSamplingClock implements SamplingClock {
TestSamplingClock(this._clock);

@override
DateTime now() => _clock.now();

@override
Stopwatch stopwatch() => _clock.stopwatch();

final Clock _clock;
}

void main() {
final TestWidgetsFlutterBinding binding = TestResampleEventFlutterBinding();
testWidgetsWithLeakTracking('PointerEvent resampling on a widget', (WidgetTester tester) async {
assert(WidgetsBinding.instance == binding);
Duration currentTestFrameTime() => Duration(milliseconds: binding.clock.now().millisecondsSinceEpoch);
Duration currentTestFrameTime() => Duration(
milliseconds: TestWidgetsFlutterBinding.instance.clock.now().millisecondsSinceEpoch,
);
void requestFrame() => SchedulerBinding.instance.scheduleFrameCallback((_) {});
final Duration epoch = currentTestFrameTime();
final ui.PointerDataPacket packet = ui.PointerDataPacket(
Expand Down
4 changes: 2 additions & 2 deletions packages/flutter/test/gestures/gesture_tester.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import 'package:fake_async/fake_async.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
import 'package:meta/meta.dart';

class GestureTester {
Expand All @@ -26,7 +26,7 @@ typedef GestureTest = void Function(GestureTester tester);

@isTest
void testGesture(String description, GestureTest callback) {
test(description, () {
testWidgetsWithLeakTracking(description, (_) async {
FakeAsync().run((FakeAsync async) {
callback(GestureTester._(async));
});
Expand Down
Loading

0 comments on commit ea3f38d

Please sign in to comment.