Skip to content

Commit

Permalink
Revert "[dart2wasm] Make int, double, bool box fields immutable"
Browse files Browse the repository at this point in the history
This reverts commit af2bea8.

Reason for revert: Caused unexpected performance regressions.

Original change's description:
> [dart2wasm] Make int, double, bool box fields immutable
>
> Make the `value` fields of `BoxedInt`, `BoxedDouble` and `BoxedBool`
> `final`, to generate immutable Wasm fields for them.
>
> Immutable fields can potentially generate better code, as loads from the
> same object will always generate the same value.
>
> To allow making the `value` fields `final`, add a constructor.
>
> To allow adding a constructor, "implement` base classes instead of
> extending them. With `extends` the front-end wants us to call the
> superclass constructors, even though they don't have any constructors.
>
> Implementing `bool` (instead of extending) causes issues in TFA as it
> currently assumes bool literals are compiled as the `bool` class. Update
> TFA to treat bool literals the same way as `int` and `double` literals.
>
> Tested: existing tests
> Change-Id: I3282e188d784fa7a22421edc79ed47f9d85faf19
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/393320
> Commit-Queue: Ömer Ağacan <omersa@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>

Change-Id: I8bb49bcc4bd9cd01f3e8a692d7ba1bef889ab555
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/393840
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
Auto-Submit: Ömer Ağacan <omersa@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
  • Loading branch information
osa1 authored and Commit Queue committed Nov 6, 2024
1 parent 74c5aa3 commit 95e7793
Show file tree
Hide file tree
Showing 8 changed files with 18 additions and 49 deletions.
5 changes: 1 addition & 4 deletions pkg/dart2wasm/lib/class_info.dart
Original file line number Diff line number Diff line change
Expand Up @@ -311,10 +311,7 @@ class ClassInfoCollector {
// directly below the public classes they implement.
// All other classes sit below their superclass.
ClassInfo superInfo = cls == translator.coreTypes.boolClass ||
cls == translator.coreTypes.numClass ||
cls == translator.boxedIntClass ||
cls == translator.boxedDoubleClass ||
cls == translator.boxedBoolClass
cls == translator.coreTypes.numClass
? topInfo
: (!translator.options.jsCompatibility &&
cls == translator.wasmStringBaseClass) ||
Expand Down
5 changes: 0 additions & 5 deletions pkg/dart2wasm/lib/target.dart
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ class WasmTarget extends Target {
Class? _closure;
Class? _boxedInt;
Class? _boxedDouble;
Class? _boxedBool;
Map<String, Class>? _nativeClasses;

@override
Expand Down Expand Up @@ -547,10 +546,6 @@ class WasmTarget extends Target {
_boxedDouble ??=
coreTypes.index.getClass("dart:_boxed_double", "BoxedDouble");

@override
Class concreteBoolLiteralClass(CoreTypes coreTypes, bool value) =>
_boxedBool ??= coreTypes.index.getClass("dart:_boxed_bool", "BoxedBool");

@override
DartLibrarySupport get dartLibrarySupport => CustomizedDartLibrarySupport(
unsupported: {if (!enableExperimentalFfi) 'ffi'});
Expand Down
2 changes: 1 addition & 1 deletion pkg/dart2wasm/lib/translator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ class Translator with KernelNodes {
late final Map<Class, Class> valueClasses = {
boxedIntClass: boxedIntClass,
boxedDoubleClass: boxedDoubleClass,
boxedBoolClass: boxedBoolClass,
boxedBoolClass: coreTypes.boolClass,
if (!options.jsCompatibility) ...{
oneByteStringClass: stringBaseClass,
twoByteStringClass: stringBaseClass
Expand Down
1 change: 0 additions & 1 deletion pkg/kernel/lib/target/targets.dart
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,6 @@ abstract class Target {
Class? concreteIntLiteralClass(CoreTypes coreTypes, int value) => null;
Class? concreteDoubleLiteralClass(CoreTypes coreTypes, double value) => null;
Class? concreteStringLiteralClass(CoreTypes coreTypes, String value) => null;
Class? concreteBoolLiteralClass(CoreTypes coreTypes, bool value) => null;

Class? concreteAsyncResultClass(CoreTypes coreTypes) => null;
Class? concreteSyncStarResultClass(CoreTypes coreTypes) => null;
Expand Down
19 changes: 4 additions & 15 deletions pkg/vm/lib/transformations/type_flow/summary_collector.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1425,6 +1425,8 @@ class SummaryCollector extends RecursiveResultVisitor<TypeExpr?> {

Class get _superclass => _staticTypeContext!.thisType!.classNode.superclass!;

Type _boolLiteralType(bool value) => value ? _boolTrue : _boolFalse;

Type _intLiteralType(int value, Constant? constant) {
final Class? concreteClass =
target.concreteIntLiteralClass(_environment.coreTypes, value);
Expand Down Expand Up @@ -1464,19 +1466,6 @@ class SummaryCollector extends RecursiveResultVisitor<TypeExpr?> {
return _stringType;
}

Type _boolLiteralType(bool value, Constant? constant) {
final Class? concreteClass =
target.concreteBoolLiteralClass(_environment.coreTypes, value);
if (concreteClass != null) {
constant ??= BoolConstant(value);
return _entryPointsListener
.addAllocatedClass(concreteClass)
.cls
.constantConcreteType(constant);
}
return value ? _boolTrue : _boolFalse;
}

TypeExpr _closureType(LocalFunction node) {
final Class? concreteClass =
target.concreteClosureClass(_environment.coreTypes);
Expand Down Expand Up @@ -1693,7 +1682,7 @@ class SummaryCollector extends RecursiveResultVisitor<TypeExpr?> {

@override
TypeExpr visitBoolLiteral(BoolLiteral node) {
return _boolLiteralType(node.value, null);
return _boolLiteralType(node.value);
}

@override
Expand Down Expand Up @@ -2856,7 +2845,7 @@ class ConstantAllocationCollector implements ConstantVisitor<Type> {

@override
Type visitBoolConstant(BoolConstant constant) {
return summaryCollector._boolLiteralType(constant.value, constant);
return summaryCollector._boolLiteralType(constant.value);
}

@override
Expand Down
19 changes: 4 additions & 15 deletions sdk/lib/_internal/wasm/lib/boxed_bool.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,13 @@
// BSD-style license that can be found in the LICENSE file.

@pragma("wasm:entry-point")
final class BoxedBool implements bool {
final class BoxedBool extends bool {
// A boxed bool contains an unboxed bool.
@pragma("wasm:entry-point")
final bool value;
bool value = false;

@pragma("wasm:entry-point")
BoxedBool._(this.value);

@override
int get hashCode => this ? 1231 : 1237;

@override
String toString() => this ? "true" : "false";
/// Dummy factory to silence error about missing superclass constructor.
external factory BoxedBool();

@override
bool operator ==(Object other) {
Expand All @@ -24,12 +18,7 @@ final class BoxedBool implements bool {
: false;
}

@override
bool operator &(bool other) => this & other; // Intrinsic &

@override
bool operator ^(bool other) => this ^ other; // Intrinsic ^

@override
bool operator |(bool other) => this | other; // Intrinsic |
}
8 changes: 4 additions & 4 deletions sdk/lib/_internal/wasm/lib/boxed_double.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ import 'dart:_string';
import 'dart:_wasm';

@pragma("wasm:entry-point")
final class BoxedDouble implements double {
final class BoxedDouble extends double {
// A boxed double contains an unboxed double.
@pragma("wasm:entry-point")
final double value;
double value = 0.0;

@pragma("wasm:entry-point")
BoxedDouble._(this.value);
/// Dummy factory to silence error about missing superclass constructor.
external factory BoxedDouble();

static const int _mantissaBits = 52;
static const int _exponentBits = 11;
Expand Down
8 changes: 4 additions & 4 deletions sdk/lib/_internal/wasm/lib/boxed_int.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import 'dart:_internal';
import 'dart:_wasm';

@pragma("wasm:entry-point")
final class BoxedInt implements int {
final class BoxedInt extends int {
// A boxed int contains an unboxed int.
@pragma("wasm:entry-point")
final int value;
int value = 0;

@pragma("wasm:entry-point")
BoxedInt._(this.value);
/// Dummy factory to silence error about missing superclass constructor.
external factory BoxedInt();

external num operator +(num other);
external num operator -(num other);
Expand Down

0 comments on commit 95e7793

Please sign in to comment.