Skip to content

Commit

Permalink
switched to a double variant of clamp to avoid boxing (#103559)
Browse files Browse the repository at this point in the history
  • Loading branch information
gaaclarke authored May 18, 2022
1 parent 32157e3 commit 64a0c19
Show file tree
Hide file tree
Showing 69 changed files with 409 additions and 193 deletions.
73 changes: 73 additions & 0 deletions dev/benchmarks/microbenchmarks/lib/foundation/clamp.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Copyright 2014 The Flutter 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:flutter/foundation.dart' show clampDouble;

import '../common.dart';

const int _kBatchSize = 100000;
const int _kNumIterations = 1000;

void main() {
assert(false,
"Don't run benchmarks in debug mode! Use 'flutter run --release'.");
final BenchmarkResultPrinter printer = BenchmarkResultPrinter();

final Stopwatch watch = Stopwatch();
{
final List<double> clampDoubleValues = <double>[];
for (int j = 0; j < _kNumIterations; ++j) {
double tally = 0;
watch.reset();
watch.start();
for (int i = 0; i < _kBatchSize; i += 1) {
tally += clampDouble(-1.0, 0.0, 1.0);
tally += clampDouble(2.0, 0.0, 1.0);
tally += clampDouble(0.0, 0.0, 1.0);
tally += clampDouble(double.nan, 0.0, 1.0);
}
watch.stop();
clampDoubleValues.add(watch.elapsedMicroseconds.toDouble() / _kBatchSize);
if (tally < 0.0) {
print("This shouldn't happen.");
}
}

printer.addResultStatistics(
description: 'clamp - clampDouble',
values: clampDoubleValues,
unit: 'us per iteration',
name: 'clamp_clampDouble',
);
}

{
final List<double> doubleClampValues = <double>[];

for (int j = 0; j < _kNumIterations; ++j) {
double tally = 0;
watch.reset();
watch.start();
for (int i = 0; i < _kBatchSize; i += 1) {
tally += -1.0.clamp(0.0, 1.0);
tally += 2.0.clamp(0.0, 1.0);
tally += 0.0.clamp(0.0, 1.0);
tally += double.nan.clamp(0.0, 1.0);
}
watch.stop();
doubleClampValues.add(watch.elapsedMicroseconds.toDouble() / _kBatchSize);
if (tally < 0.0) {
print("This shouldn't happen.");
}
}

printer.addResultStatistics(
description: 'clamp - Double.clamp',
values: doubleClampValues,
unit: 'us per iteration',
name: 'clamp_Double_clamp',
);
}
printer.printToStdout();
}
96 changes: 85 additions & 11 deletions dev/bots/analyze.dart
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ Future<void> run(List<String> arguments) async {
exitWithError(<String>['The analyze.dart script must be run with --enable-asserts.']);
}

print('$clock No Double.clamp');
await verifyNoDoubleClamp(flutterRoot);

print('$clock All tool test files end in _test.dart...');
await verifyToolTestsEndInTestDart(flutterRoot);

Expand Down Expand Up @@ -203,6 +206,80 @@ Future<void> run(List<String> arguments) async {

// TESTS

FeatureSet _parsingFeatureSet() => FeatureSet.fromEnableFlags2(
sdkLanguageVersion: Version.parse('2.17.0-0'),
flags: <String>['super-parameters']);

_Line _getLine(ParseStringResult parseResult, int offset) {
final int lineNumber =
parseResult.lineInfo.getLocation(offset).lineNumber;
final String content = parseResult.content.substring(
parseResult.lineInfo.getOffsetOfLine(lineNumber - 1),
parseResult.lineInfo.getOffsetOfLine(lineNumber) - 1);
return _Line(lineNumber, content);
}

class _DoubleClampVisitor extends RecursiveAstVisitor<CompilationUnit> {
_DoubleClampVisitor(this.parseResult);

final List<_Line> clamps = <_Line>[];
final ParseStringResult parseResult;

@override
CompilationUnit? visitMethodInvocation(MethodInvocation node) {
if (node.methodName.name == 'clamp') {
final _Line line = _getLine(parseResult, node.function.offset);
if (!line.content.contains('// ignore_clamp_double_lint')) {
clamps.add(line);
}
}

node.visitChildren(this);
return null;
}
}

/// Verify that we use clampDouble instead of Double.clamp for performance reasons.
///
/// We currently can't distinguish valid uses of clamp from problematic ones so
/// if the clamp is operating on a type other than a `double` the
/// `// ignore_clamp_double_lint` comment must be added to the line where clamp is
/// invoked.
///
/// See also:
/// * https://github.com/flutter/flutter/pull/103559
/// * https://github.com/flutter/flutter/issues/103917
Future<void> verifyNoDoubleClamp(String workingDirectory) async {
final String flutterLibPath = '$workingDirectory/packages/flutter/lib';
final Stream<File> testFiles =
_allFiles(flutterLibPath, 'dart', minimumMatches: 100);
final List<String> errors = <String>[];
await for (final File file in testFiles) {
try {
final ParseStringResult parseResult = parseFile(
featureSet: _parsingFeatureSet(),
path: file.absolute.path,
);
final _DoubleClampVisitor visitor = _DoubleClampVisitor(parseResult);
visitor.visitCompilationUnit(parseResult.unit);
for (final _Line clamp in visitor.clamps) {
errors.add('${file.path}:${clamp.line}: `clamp` method used without ignore_clamp_double_lint comment.');
}
} catch (ex) {
// TODO(gaaclarke): There is a bug with super parameter parsing on mac so
// we skip certain files until that is fixed.
// https://github.com/dart-lang/sdk/issues/49032
print('skipping ${file.path}: $ex');
}
}
if (errors.isNotEmpty) {
exitWithError(<String>[
...errors,
'\n${bold}See: https://github.com/flutter/flutter/pull/103559',
]);
}
}

/// Verify tool test files end in `_test.dart`.
///
/// The test runner will only recognize files ending in `_test.dart` as tests to
Expand Down Expand Up @@ -518,16 +595,16 @@ Future<int> _verifyNoMissingLicenseForExtension(
return 0;
}

class _TestSkip {
_TestSkip(this.line, this.content);
class _Line {
_Line(this.line, this.content);

final int line;
final String content;
}

Iterable<_TestSkip> _getTestSkips(File file) {
Iterable<_Line> _getTestSkips(File file) {
final ParseStringResult parseResult = parseFile(
featureSet: FeatureSet.fromEnableFlags2(sdkLanguageVersion: Version.parse('2.17.0-0'), flags: <String>['super-parameters']),
featureSet: _parsingFeatureSet(),
path: file.absolute.path,
);
final _TestSkipLinesVisitor<CompilationUnit> visitor = _TestSkipLinesVisitor<CompilationUnit>(parseResult);
Expand All @@ -536,10 +613,10 @@ Iterable<_TestSkip> _getTestSkips(File file) {
}

class _TestSkipLinesVisitor<T> extends RecursiveAstVisitor<T> {
_TestSkipLinesVisitor(this.parseResult) : skips = <_TestSkip>{};
_TestSkipLinesVisitor(this.parseResult) : skips = <_Line>{};

final ParseStringResult parseResult;
final Set<_TestSkip> skips;
final Set<_Line> skips;

static bool isTestMethod(String name) {
return name.startsWith('test') || name == 'group' || name == 'expect';
Expand All @@ -550,10 +627,7 @@ class _TestSkipLinesVisitor<T> extends RecursiveAstVisitor<T> {
if (isTestMethod(node.methodName.toString())) {
for (final Expression argument in node.argumentList.arguments) {
if (argument is NamedExpression && argument.name.label.name == 'skip') {
final int lineNumber = parseResult.lineInfo.getLocation(argument.beginToken.charOffset).lineNumber;
final String content = parseResult.content.substring(parseResult.lineInfo.getOffsetOfLine(lineNumber - 1),
parseResult.lineInfo.getOffsetOfLine(lineNumber) - 1);
skips.add(_TestSkip(lineNumber, content));
skips.add(_getLine(parseResult, argument.beginToken.charOffset));
}
}
}
Expand All @@ -571,7 +645,7 @@ Future<void> verifySkipTestComments(String workingDirectory) async {
.where((File f) => f.path.endsWith('_test.dart'));

await for (final File file in testFiles) {
for (final _TestSkip skip in _getTestSkips(file)) {
for (final _Line skip in _getTestSkips(file)) {
final Match? match = _skipTestCommentPattern.firstMatch(skip.content);
final String? skipComment = match?.group(1);
if (skipComment == null ||
Expand Down
1 change: 1 addition & 0 deletions dev/devicelab/lib/tasks/microbenchmarks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ TaskFunction createMicrobenchmarkTask() {
...await runMicrobench('lib/language/sync_star_bench.dart'),
...await runMicrobench('lib/language/sync_star_semantics_bench.dart'),
...await runMicrobench('lib/foundation/all_elements_bench.dart'),
...await runMicrobench('lib/foundation/clamp.dart'),
...await runMicrobench('lib/foundation/change_notifier_bench.dart'),
...await runMicrobench('lib/foundation/standard_method_codec_bench.dart'),
...await runMicrobench('lib/foundation/standard_message_codec_bench.dart'),
Expand Down
1 change: 1 addition & 0 deletions packages/flutter/lib/foundation.dart
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export 'src/foundation/diagnostics.dart';
export 'src/foundation/isolates.dart';
export 'src/foundation/key.dart';
export 'src/foundation/licenses.dart';
export 'src/foundation/math.dart';
export 'src/foundation/node.dart';
export 'src/foundation/object.dart';
export 'src/foundation/observer_list.dart';
Expand Down
10 changes: 5 additions & 5 deletions packages/flutter/lib/src/animation/animation_controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ class AnimationController extends Animation<double>
}

void _internalSetValue(double newValue) {
_value = newValue.clamp(lowerBound, upperBound);
_value = clampDouble(newValue, lowerBound, upperBound);
if (_value == lowerBound) {
_status = AnimationStatus.dismissed;
} else if (_value == upperBound) {
Expand Down Expand Up @@ -598,7 +598,7 @@ class AnimationController extends Animation<double>
stop();
if (simulationDuration == Duration.zero) {
if (value != target) {
_value = target.clamp(lowerBound, upperBound);
_value = clampDouble(target, lowerBound, upperBound);
notifyListeners();
}
_status = (_direction == _AnimationDirection.forward) ?
Expand Down Expand Up @@ -741,7 +741,7 @@ class AnimationController extends Animation<double>
assert(!isAnimating);
_simulation = simulation;
_lastElapsedDuration = Duration.zero;
_value = simulation.x(0.0).clamp(lowerBound, upperBound);
_value = clampDouble(simulation.x(0.0), lowerBound, upperBound);
final TickerFuture result = _ticker!.start();
_status = (_direction == _AnimationDirection.forward) ?
AnimationStatus.forward :
Expand Down Expand Up @@ -820,7 +820,7 @@ class AnimationController extends Animation<double>
_lastElapsedDuration = elapsed;
final double elapsedInSeconds = elapsed.inMicroseconds.toDouble() / Duration.microsecondsPerSecond;
assert(elapsedInSeconds >= 0.0);
_value = _simulation!.x(elapsedInSeconds).clamp(lowerBound, upperBound);
_value = clampDouble(_simulation!.x(elapsedInSeconds), lowerBound, upperBound);
if (_simulation!.isDone(elapsedInSeconds)) {
_status = (_direction == _AnimationDirection.forward) ?
AnimationStatus.completed :
Expand Down Expand Up @@ -855,7 +855,7 @@ class _InterpolationSimulation extends Simulation {

@override
double x(double timeInSeconds) {
final double t = (timeInSeconds / _durationInSeconds).clamp(0.0, 1.0);
final double t = clampDouble(timeInSeconds / _durationInSeconds, 0.0, 1.0);
if (t == 0.0)
return _begin;
else if (t == 1.0)
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter/lib/src/animation/curves.dart
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class Interval extends Curve {
assert(end >= 0.0);
assert(end <= 1.0);
assert(end >= begin);
t = ((t - begin) / (end - begin)).clamp(0.0, 1.0);
t = clampDouble((t - begin) / (end - begin), 0.0, 1.0);
if (t == 0.0 || t == 1.0)
return t;
return curve.transform(t);
Expand Down
3 changes: 2 additions & 1 deletion packages/flutter/lib/src/cupertino/context_menu.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'dart:math' as math;
import 'dart:ui' as ui;

import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart' show kMinFlingVelocity, kLongPressTimeout;
import 'package:flutter/scheduler.dart';
Expand Down Expand Up @@ -958,7 +959,7 @@ class _ContextMenuRouteStaticState extends State<_ContextMenuRouteStatic> with T
_moveAnimation = Tween<Offset>(
begin: Offset.zero,
end: Offset(
endX.clamp(-_kPadding, _kPadding),
clampDouble(endX, -_kPadding, _kPadding),
endY,
),
).animate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/foundation.dart' show clampDouble;
import 'package:flutter/gestures.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/widgets.dart';
Expand Down Expand Up @@ -157,7 +158,7 @@ class _CupertinoDesktopTextSelectionControlsToolbarState extends State<_Cupertin
final MediaQueryData mediaQuery = MediaQuery.of(context);

final Offset midpointAnchor = Offset(
(widget.selectionMidpoint.dx - widget.globalEditableRegion.left).clamp(
clampDouble(widget.selectionMidpoint.dx - widget.globalEditableRegion.left,
mediaQuery.padding.left,
mediaQuery.size.width - mediaQuery.padding.right,
),
Expand Down
3 changes: 2 additions & 1 deletion packages/flutter/lib/src/cupertino/refresh.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'dart:math';

import 'package:flutter/foundation.dart' show clampDouble;
import 'package:flutter/rendering.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/services.dart';
Expand Down Expand Up @@ -375,7 +376,7 @@ class CupertinoSliverRefreshControl extends StatefulWidget {
double refreshTriggerPullDistance,
double refreshIndicatorExtent,
) {
final double percentageComplete = (pulledExtent / refreshTriggerPullDistance).clamp(0.0, 1.0);
final double percentageComplete = clampDouble(pulledExtent / refreshTriggerPullDistance, 0.0, 1.0);

// Place the indicator at the top of the sliver that opens up. Note that we're using
// a Stack/Positioned widget because the CupertinoActivityIndicator does some internal
Expand Down
11 changes: 6 additions & 5 deletions packages/flutter/lib/src/cupertino/slider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'dart:math' as math;
import 'dart:ui' show lerpDouble;

import 'package:flutter/foundation.dart' show clampDouble;
import 'package:flutter/gestures.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/widgets.dart';
Expand Down Expand Up @@ -434,7 +435,7 @@ class _RenderCupertinoSlider extends RenderConstrainedBox {
double _currentDragValue = 0.0;

double get _discretizedCurrentDragValue {
double dragValue = _currentDragValue.clamp(0.0, 1.0);
double dragValue = clampDouble(_currentDragValue, 0.0, 1.0);
if (divisions != null)
dragValue = (dragValue * divisions!).round() / divisions!;
return dragValue;
Expand Down Expand Up @@ -554,20 +555,20 @@ class _RenderCupertinoSlider extends RenderConstrainedBox {
config.onIncrease = _increaseAction;
config.onDecrease = _decreaseAction;
config.value = '${(value * 100).round()}%';
config.increasedValue = '${((value + _semanticActionUnit).clamp(0.0, 1.0) * 100).round()}%';
config.decreasedValue = '${((value - _semanticActionUnit).clamp(0.0, 1.0) * 100).round()}%';
config.increasedValue = '${(clampDouble(value + _semanticActionUnit, 0.0, 1.0) * 100).round()}%';
config.decreasedValue = '${(clampDouble(value - _semanticActionUnit, 0.0, 1.0) * 100).round()}%';
}
}

double get _semanticActionUnit => divisions != null ? 1.0 / divisions! : _kAdjustmentUnit;

void _increaseAction() {
if (isInteractive)
onChanged!((value + _semanticActionUnit).clamp(0.0, 1.0));
onChanged!(clampDouble(value + _semanticActionUnit, 0.0, 1.0));
}

void _decreaseAction() {
if (isInteractive)
onChanged!((value - _semanticActionUnit).clamp(0.0, 1.0));
onChanged!(clampDouble(value - _semanticActionUnit, 0.0, 1.0));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ class _SegmentedControlState<T> extends State<CupertinoSlidingSegmentedControl<T
final int numOfChildren = widget.children.length;
assert(renderBox.hasSize);
assert(numOfChildren >= 2);
int index = (dx ~/ (renderBox.size.width / numOfChildren)).clamp(0, numOfChildren - 1);
int index = (dx ~/ (renderBox.size.width / numOfChildren)).clamp(0, numOfChildren - 1); // ignore_clamp_double_lint

switch (Directionality.of(context)) {
case TextDirection.ltr:
Expand Down
Loading

0 comments on commit 64a0c19

Please sign in to comment.