From 15792df87e2f5172890337fa0f8c255192dcbbf0 Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Mon, 15 Jul 2024 02:18:52 +0000 Subject: [PATCH] [dart2js] Use indexes for operation names and verbs Change-Id: Ic954a7e20062aa8bf1c0f622ee9b0656bc563ba5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/375024 Commit-Queue: Stephen Adams Reviewed-by: Mayank Patke --- pkg/compiler/lib/src/ssa/builder.dart | 5 +- pkg/compiler/lib/src/ssa/codegen.dart | 23 ++--- .../src/ssa/invoke_dynamic_specializers.dart | 7 +- pkg/compiler/lib/src/ssa/nodes.dart | 30 ++++++- pkg/compiler/lib/src/ssa/optimize.dart | 51 +++++++++++ .../data/array_flags_optimizations.dart | 90 +++++++++++++++++++ .../codegen/data/unmodifiable_bytedata.dart | 6 +- pkg/compiler/test/rti/emission/list.dart | 2 +- pkg/js_runtime/lib/synced/array_flags.dart | 60 +++++++++++++ .../js_runtime/lib/foreign_helper.dart | 8 +- .../_internal/js_runtime/lib/js_array.dart | 45 +++++----- .../_internal/js_runtime/lib/js_helper.dart | 69 ++++++++++---- .../js_runtime/lib/synced/array_flags.dart | 60 +++++++++++++ .../web/internal/array_flags_errors_test.dart | 34 +++++++ tests/web/web.status | 7 +- 15 files changed, 429 insertions(+), 68 deletions(-) create mode 100644 pkg/compiler/test/codegen/data/array_flags_optimizations.dart create mode 100644 tests/web/internal/array_flags_errors_test.dart diff --git a/pkg/compiler/lib/src/ssa/builder.dart b/pkg/compiler/lib/src/ssa/builder.dart index 569241aceb7b..57bff7fbba77 100644 --- a/pkg/compiler/lib/src/ssa/builder.dart +++ b/pkg/compiler/lib/src/ssa/builder.dart @@ -5424,7 +5424,7 @@ class KernelSsaGraphBuilder extends ir.VisitorDefault void _handleArrayFlagsCheck( ir.StaticInvocation invocation, SourceInformation? sourceInformation) { if (_unexpectedForeignArguments(invocation, - minPositional: 4, maxPositional: 4, typeArgumentCount: 1)) { + minPositional: 4, maxPositional: 5, typeArgumentCount: 1)) { // Result expected on stack. stack.add(graph.addConstantNull(closedWorld)); return; @@ -5434,6 +5434,7 @@ class KernelSsaGraphBuilder extends ir.VisitorDefault final arrayFlags = inputs[1]; final checkFlags = inputs[2]; final operation = inputs[3]; + final verb = inputs.length > 4 ? inputs[4] : null; // TODO(sra): Use the flags to improve in the AbstractValue, which may // contain powerset domain bits outside of the conventional type @@ -5444,7 +5445,7 @@ class KernelSsaGraphBuilder extends ir.VisitorDefault instructionType ??= _abstractValueDomain.dynamicType; push(HArrayFlagsCheck( - array, arrayFlags, checkFlags, operation, instructionType) + array, arrayFlags, checkFlags, operation, verb, instructionType) ..sourceInformation = sourceInformation); } diff --git a/pkg/compiler/lib/src/ssa/codegen.dart b/pkg/compiler/lib/src/ssa/codegen.dart index 4fa574d8f25e..c2cb9f482b39 100644 --- a/pkg/compiler/lib/src/ssa/codegen.dart +++ b/pkg/compiler/lib/src/ssa/codegen.dart @@ -3486,18 +3486,21 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor { test = js.js('# & #', [arrayFlags, checkFlags]); } - final operation = node.operation; - // Most common operation is "[]=", so 'pass' that by leaving it out. - if (operation - case HConstant(constant: StringConstantValue(stringValue: '[]='))) { - _pushCallStatic(_commonElements.throwUnsupportedOperation, [array], - node.sourceInformation); - } else { - use(operation); - _pushCallStatic(_commonElements.throwUnsupportedOperation, [array, pop()], - node.sourceInformation); + List arguments = [array]; + + if (node.hasOperation) { + use(node.operation); + arguments.add(pop()); } + if (node.hasVerb) { + use(node.verb); + arguments.add(pop()); + } + + _pushCallStatic(_commonElements.throwUnsupportedOperation, arguments, + node.sourceInformation); + js.Statement check; if (test == null) { check = js.js.statement('#;', pop()); diff --git a/pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart b/pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart index 0e6fc59eeccd..b1fa772eadb0 100644 --- a/pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart +++ b/pkg/compiler/lib/src/ssa/invoke_dynamic_specializers.dart @@ -246,10 +246,11 @@ class IndexAssignSpecializer extends InvokeDynamicSpecializer { HInstruction mask = graph.addConstantInt(ArrayFlags.unmodifiableCheck, closedWorld); HInstruction name = graph.addConstantString('[]=', closedWorld); + HInstruction verb = graph.addConstantString('modify', closedWorld); final instructionType = receiver.instructionType; - final checkFlags = - HArrayFlagsCheck(receiver, getFlags, mask, name, instructionType) - ..sourceInformation = instruction.sourceInformation; + final checkFlags = HArrayFlagsCheck( + receiver, getFlags, mask, name, verb, instructionType) + ..sourceInformation = instruction.sourceInformation; instruction.block!.addBefore(instruction, checkFlags); checkFlags.instructionType = checkFlags.computeInstructionType( instructionType, abstractValueDomain); diff --git a/pkg/compiler/lib/src/ssa/nodes.dart b/pkg/compiler/lib/src/ssa/nodes.dart index e55719b13bda..45b29045fee9 100644 --- a/pkg/compiler/lib/src/ssa/nodes.dart +++ b/pkg/compiler/lib/src/ssa/nodes.dart @@ -1401,6 +1401,12 @@ abstract class HInstruction implements SpannableWithEntity { replacement.usedBy.add(this); } + /// Remove a single input. + void removeInput(int index) { + inputs[index].usedBy.remove(this); + inputs.removeAt(index); + } + void replaceAllUsersDominatedBy( HInstruction cursor, HInstruction newInstruction) { DominatedUses.of(this, cursor).replaceWith(newInstruction); @@ -4707,21 +4713,37 @@ class HTypeBind extends HInstruction implements HRtiInstruction { /// /// a = ... /// f = HArrayFlagsGet(a); -/// a2 = HArrayFlagsCheck(a, f, ArrayFlags.unmodifiableCheck, "[]=") +/// a2 = HArrayFlagsCheck(a, f, ArrayFlags.unmodifiableCheck, "[]=", "modify") /// a2[i] = 0 /// /// HArrayFlagsGet is a separate instruction so that 'loading' the flags from /// the Array can by hoisted. class HArrayFlagsCheck extends HCheck { - HArrayFlagsCheck(HInstruction array, HInstruction arrayFlags, - HInstruction checkFlags, HInstruction operation, AbstractValue type) - : super([array, arrayFlags, checkFlags, operation], type); + HArrayFlagsCheck( + HInstruction array, + HInstruction arrayFlags, + HInstruction checkFlags, + HInstruction? operation, + HInstruction? verb, + AbstractValue type) + : super([ + array, + arrayFlags, + checkFlags, + if (operation != null) operation, + if (verb != null) verb, + ], type); HInstruction get array => inputs[0]; HInstruction get arrayFlags => inputs[1]; HInstruction get checkFlags => inputs[2]; + + bool get hasOperation => inputs.length > 3; HInstruction get operation => inputs[3]; + bool get hasVerb => inputs.length > 4; + HInstruction get verb => inputs[4]; + // The checked type is the input type, refined to match the flags. AbstractValue computeInstructionType( AbstractValue inputType, AbstractValueDomain domain) { diff --git a/pkg/compiler/lib/src/ssa/optimize.dart b/pkg/compiler/lib/src/ssa/optimize.dart index 257083339abb..74e7f215ca92 100644 --- a/pkg/compiler/lib/src/ssa/optimize.dart +++ b/pkg/compiler/lib/src/ssa/optimize.dart @@ -2675,6 +2675,57 @@ class SsaInstructionSimplifier extends HBaseVisitor } } + // The 'operation' and 'verb' strings can be replaced with an index into a + // small table to known operations or verbs. This makes the call-sites + // smaller, so is worthwhile for calls to HArrayFlagsCheck that are inlined + // into multiple places. + // + // A trailing zero index (verb, or verb and operation) can be omitted from + // the instruction. + // + // When both indexes are replaced by indexes, the indexes are combined into + // a single value. + // + // finalIndex = verbIndex * numberOfOperationIndexes + operationIndex + + int? verbIndex; // Verb index if nonzero. + + if (node.hasVerb) { + if (node.verb + case HConstant(constant: StringConstantValue(:final stringValue))) { + final index = ArrayFlags.verbToIndex[stringValue]; + if (index != null) { + if (index == 0) { + node.removeInput(4); + } else { + final replacement = _graph.addConstantInt(index, _closedWorld); + node.replaceInput(4, replacement); + verbIndex = index; + } + } + } + } + + if (node.hasOperation) { + if (node.operation + case HConstant(constant: StringConstantValue(:final stringValue))) { + var index = ArrayFlags.operationNameToIndex[stringValue]; + if (index != null) { + if (index == 0 && !node.hasVerb) { + node.removeInput(3); + } else { + if (verbIndex != null) { + // Encode combined indexes and remove 'verb' input. + index += verbIndex * ArrayFlags.operationNameToIndex.length; + node.removeInput(4); + } + final replacement = _graph.addConstantInt(index, _closedWorld); + node.replaceInput(3, replacement); + } + } + } + } + return node; } diff --git a/pkg/compiler/test/codegen/data/array_flags_optimizations.dart b/pkg/compiler/test/codegen/data/array_flags_optimizations.dart new file mode 100644 index 000000000000..5030fb17e068 --- /dev/null +++ b/pkg/compiler/test/codegen/data/array_flags_optimizations.dart @@ -0,0 +1,90 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Test of various space-saving contractions of calls to +// `throwUnsupportedOperation`. + +import 'dart:_foreign_helper' show ArrayFlags, HArrayFlagsCheck; + +@pragma('dart2js:never-inline') +// The operation and verb are both elided. +/*member: indexSetter:function() { + A.throwUnsupportedOperation(B.List_empty); +}*/ +indexSetter() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + '[]=', 'modify'); +} + +@pragma('dart2js:never-inline') +// The operation is reduced to an index but cannot be elided because it is +// followed by a non-elided verb. +/*member: indexSetterUnusualVerb:function() { + A.throwUnsupportedOperation(B.List_empty, 0, "change contents of"); +}*/ +indexSetterUnusualVerb() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + '[]=', 'change contents of'); +} + +@pragma('dart2js:never-inline') +// The verb is elided. +/*member: unusualOperationModify:function() { + A.throwUnsupportedOperation(B.List_empty, "rub"); +}*/ +unusualOperationModify() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + 'rub', 'modify'); +} + +@pragma('dart2js:never-inline') +// The operation is left as a string and verb is +/*member: unusualOperationRemoveFrom:function() { + A.throwUnsupportedOperation(B.List_empty, "rub", 1); +}*/ +unusualOperationRemoveFrom() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + 'rub', 'remove from'); +} + +@pragma('dart2js:never-inline') +// The operation and verb are left as strings. +/*member: unusualOperationAndVerb:function() { + A.throwUnsupportedOperation(B.List_empty, "rub", "burnish"); +}*/ +unusualOperationAndVerb() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + 'rub', 'burnish'); +} + +@pragma('dart2js:never-inline') +// The operation is reduced to an index and the verb is elided. +/*member: knownOperationModify:function() { + A.throwUnsupportedOperation(B.List_empty, 10); +}*/ +knownOperationModify() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + 'setUint16', 'modify'); +} + +@pragma('dart2js:never-inline') +// The operation and verb are combined to a single number. +/*member: knownOperationAndVerb:function() { + A.throwUnsupportedOperation(B.List_empty, 16); +}*/ +knownOperationAndVerb() { + HArrayFlagsCheck(const [], ArrayFlags.constant, ArrayFlags.unmodifiableCheck, + 'removeWhere', 'remove from'); +} + +/*member: main:ignore*/ +main() { + indexSetter(); + indexSetterUnusualVerb(); + unusualOperationModify(); + unusualOperationRemoveFrom(); + unusualOperationAndVerb(); + knownOperationModify(); + knownOperationAndVerb(); +} diff --git a/pkg/compiler/test/codegen/data/unmodifiable_bytedata.dart b/pkg/compiler/test/codegen/data/unmodifiable_bytedata.dart index 84b30543c511..3a5ae282b537 100644 --- a/pkg/compiler/test/codegen/data/unmodifiable_bytedata.dart +++ b/pkg/compiler/test/codegen/data/unmodifiable_bytedata.dart @@ -52,7 +52,7 @@ returnModifiable2() { /*member: guaranteedFail:function() { var a = new DataView(new ArrayBuffer(10)); a.$flags = 3; - A.throwUnsupportedOperation(a, "setInt32"); + A.throwUnsupportedOperation(a, 8); a.setInt32(0, 100, false); a.setUint32(4, 2000, false); return a; @@ -67,7 +67,7 @@ guaranteedFail() { @pragma('dart2js:never-inline') /*member: multipleWrites:function(data) { - data.$flags & 2 && A.throwUnsupportedOperation(data, "setFloat64"); + data.$flags & 2 && A.throwUnsupportedOperation(data, 13); data.setFloat64(0, 1.23, false); data.setFloat32(8, 1.23, false); return data; @@ -83,7 +83,7 @@ multipleWrites(ByteData data) { /*member: hoistedLoad:function(data) { var t1, i; for (t1 = data.$flags | 0, i = 0; i < data.byteLength; i += 2) { - t1 & 2 && A.throwUnsupportedOperation(data, "setUint16"); + t1 & 2 && A.throwUnsupportedOperation(data, 10); data.setUint16(i, 100, true); } return data; diff --git a/pkg/compiler/test/rti/emission/list.dart b/pkg/compiler/test/rti/emission/list.dart index 426fc3467d9d..fc07d6c000a4 100644 --- a/pkg/compiler/test/rti/emission/list.dart +++ b/pkg/compiler/test/rti/emission/list.dart @@ -2,7 +2,7 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -/*spec.class: global#JSArray:checkedInstance,checks=[$isIterable],instance*/ +/*spec.class: global#JSArray:checkedInstance,checks=[$isIterable,$isList],instance*/ /*prod.class: global#JSArray:checks=[$isIterable],instance*/ /*class: global#Iterable:checkedInstance*/ diff --git a/pkg/js_runtime/lib/synced/array_flags.dart b/pkg/js_runtime/lib/synced/array_flags.dart index d8ca41ea66f5..319b73729793 100644 --- a/pkg/js_runtime/lib/synced/array_flags.dart +++ b/pkg/js_runtime/lib/synced/array_flags.dart @@ -60,4 +60,64 @@ class ArrayFlags { /// Bit to check for constant JSArray. static const int constantCheck = 4; + + /// A List of operation names for HArrayFlagsCheck, encoded in a string of + /// names separated by semicolons. + /// + /// The optimizer may replace the string with an index into this + /// table. Generally this makes the generated code smaller. A name does not + /// need to be in this table. It makes sense for this table to include only + /// names that occur in more than one HArrayFlagsCheck, either as written in + /// js_runtime, or occuring multiple times via inlining. + static const operationNames = // + '[]=' // This is the most common operation so it comes first. + ';add' + ';removeWhere' + ';retainWhere' + ';removeRange' + ';setRange' + ';setInt8' + ';setInt16' + ';setInt32' + ';setUint8' + ';setUint16' + ';setUint32' + ';setFloat32' + ';setFloat64' + // + ; + + /// A list of 'verbs' for HArrayFlagsCheck, encoded as a string of phrases + /// separated by semicolons. The 'verb' fills a span of the error message, for + /// example, "remove from" in the message + /// + /// Cannot remove from an unmodifiable list. + /// ^^^^^^^^^^^ + /// + /// The optimizer may replace the string with an index into this + /// table. Generally this makes the program smaller. A 'verb' does not need to + /// be in this table. It makes sense to only have verbs that are used by + /// several calls to HArrayFlagsCheck, either as written in js_runtime, or + /// occuring multiple times via inlining. + static const verbs = // + 'modify' // This is the verb for '[]=' so it comes first. + ';remove from' + ';add to' + // + ; + + /// A view of [operationNames] as a map from the name to the index of the name + /// in the list. + static final Map operationNameToIndex = _invert(operationNames); + + /// A view of [verbs] as a map from the verb to the index of the verb in the + /// list. + static final Map verbToIndex = _invert(verbs); + + static Map _invert(String joinedStrings) { + return { + for (final entry in joinedStrings.split(';').asMap().entries) + entry.value: entry.key, + }; + } } diff --git a/sdk/lib/_internal/js_runtime/lib/foreign_helper.dart b/sdk/lib/_internal/js_runtime/lib/foreign_helper.dart index 6d01aabc2ba4..95907cb0ee99 100644 --- a/sdk/lib/_internal/js_runtime/lib/foreign_helper.dart +++ b/sdk/lib/_internal/js_runtime/lib/foreign_helper.dart @@ -336,11 +336,15 @@ external int HArrayFlagsGet(Object array); /// intervening HArrayFlagsSet that might invalidate the check. @pragma('dart2js:as:trust') T HArrayFlagsCheck( - Object array, int arrayFlags, int checkFlags, String operation) { + Object array, int arrayFlags, int checkFlags, String operation, + [String verb = 'modify']) { // This body is unused but serves as a model for global for impacts and // analysis. if (arrayFlags & checkFlags != 0) { - throwUnsupportedOperation(array, operation); + if (operation == '[]=') throwUnsupportedOperation(array); + if (operation == 'setUint32') throwUnsupportedOperation(array, 1); + if (verb == 'remove from') throwUnsupportedOperation(array, operation, 1); + throwUnsupportedOperation(array, operation, verb); } return array as T; } diff --git a/sdk/lib/_internal/js_runtime/lib/js_array.dart b/sdk/lib/_internal/js_runtime/lib/js_array.dart index 03ccb142f0fb..1ca055c0ca9d 100644 --- a/sdk/lib/_internal/js_runtime/lib/js_array.dart +++ b/sdk/lib/_internal/js_runtime/lib/js_array.dart @@ -146,24 +146,25 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { return !isUnmodifiable(a); } - checkMutable(String reason) { + checkMutable(String operation, String verb) { final int flags = HArrayFlagsGet(this); - HArrayFlagsCheck(this, flags, ArrayFlags.unmodifiableCheck, reason); + HArrayFlagsCheck( + this, flags, ArrayFlags.unmodifiableCheck, operation, verb); } - checkGrowable(String reason) { + checkGrowable(String operation, String verb) { final int flags = HArrayFlagsGet(this); - HArrayFlagsCheck(this, flags, ArrayFlags.fixedLengthCheck, reason); + HArrayFlagsCheck(this, flags, ArrayFlags.fixedLengthCheck, operation, verb); } List cast() => List.castFrom(this); void add(E value) { - checkGrowable('add'); + checkGrowable('add', 'add to'); JS('void', r'#.push(#)', this, value); } E removeAt(int index) { - checkGrowable('removeAt'); + checkGrowable('removeAt', 'remove from'); if (index is! int) throw argumentErrorValue(index); if (index < 0 || index >= length) { throw RangeError.value(index); @@ -172,7 +173,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void insert(int index, E value) { - checkGrowable('insert'); + checkGrowable('insert', 'add to'); if (index is! int) throw argumentErrorValue(index); if (index < 0 || index > length) { throw RangeError.value(index); @@ -181,7 +182,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void insertAll(int index, Iterable iterable) { - checkGrowable('insertAll'); + checkGrowable('insertAll', 'add to'); RangeError.checkValueInInterval(index, 0, this.length, 'index'); if (iterable is! EfficientLengthIterable) { iterable = iterable.toList(); @@ -194,7 +195,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void setAll(int index, Iterable iterable) { - checkMutable('setAll'); + checkMutable('setAll', 'modify'); RangeError.checkValueInInterval(index, 0, this.length, 'index'); for (var element in iterable) { this[index++] = element; @@ -202,13 +203,13 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } E removeLast() { - checkGrowable('removeLast'); + checkGrowable('removeLast', 'remove from'); if (length == 0) throw diagnoseIndexError(this, -1); return JS('', r'#.pop()', this); } bool remove(Object? element) { - checkGrowable('remove'); + checkGrowable('remove', 'remove from'); for (int i = 0; i < this.length; i++) { if (this[i] == element) { JS('', r'#.splice(#, 1)', this, i); @@ -220,12 +221,12 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { /// Removes elements matching [test] from this [JSArray]. void removeWhere(bool test(E element)) { - checkGrowable('removeWhere'); + checkGrowable('removeWhere', 'remove from'); _removeWhere(test, true); } void retainWhere(bool test(E element)) { - checkGrowable('retainWhere'); + checkGrowable('retainWhere', 'remove from'); _removeWhere(test, false); } @@ -266,7 +267,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void addAll(Iterable collection) { - checkGrowable('addAll'); + checkGrowable('addAll', 'add to'); if (collection is JSArray) { _addAllFromArray(JS('', '#', collection)); return; @@ -289,7 +290,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { @pragma('dart2js:noInline') void clear() { - checkGrowable('clear'); + checkGrowable('clear', 'clear'); _clear(); } @@ -459,14 +460,14 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void removeRange(int start, int end) { - checkGrowable('removeRange'); + checkGrowable('removeRange', 'remove from'); RangeError.checkValidRange(start, end, this.length); int deleteCount = end - start; JS('', '#.splice(#, #)', this, start, deleteCount); } void setRange(int start, int end, Iterable iterable, [int skipCount = 0]) { - checkMutable('setRange'); + checkMutable('setRange', 'modify'); RangeError.checkValidRange(start, end, this.length); int length = end - start; @@ -506,7 +507,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void fillRange(int start, int end, [E? fillValue]) { - checkMutable('fill range'); + checkMutable('fillRange', 'modify'); RangeError.checkValidRange(start, end, this.length); E checkedFillValue = fillValue as E; for (int i = start; i < end; i++) { @@ -517,7 +518,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void replaceRange(int start, int end, Iterable replacement) { - checkGrowable('replaceRange'); + checkGrowable('replaceRange', 'remove from or add to'); RangeError.checkValidRange(start, end, this.length); if (replacement is! EfficientLengthIterable) { replacement = replacement.toList(); @@ -570,7 +571,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { Iterable get reversed => ReversedListIterable(this); void sort([int Function(E, E)? compare]) { - checkMutable('sort'); + checkMutable('sort', 'modify'); final len = length; if (len < 2) return; compare ??= _compareAny; @@ -664,7 +665,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { } void shuffle([Random? random]) { - checkMutable('shuffle'); + checkMutable('shuffle', 'modify'); if (random == null) random = Random(); int length = this.length; while (length > 1) { @@ -740,7 +741,7 @@ class JSArray extends JavaScriptObject implements List, JSIndexable { int get length => JS('JSUInt32', r'#.length', this); set length(int newLength) { - checkGrowable('set length'); + checkGrowable('set length', 'change the length of'); if (newLength is! int) { throw ArgumentError.value(newLength, 'newLength'); } diff --git a/sdk/lib/_internal/js_runtime/lib/js_helper.dart b/sdk/lib/_internal/js_runtime/lib/js_helper.dart index 13d35a88a8a8..37c50335a29a 100644 --- a/sdk/lib/_internal/js_runtime/lib/js_helper.dart +++ b/sdk/lib/_internal/js_runtime/lib/js_helper.dart @@ -1253,43 +1253,76 @@ throwUnsupportedError(message) { /// Called from code generated for the HArrayFlagsCheck instruction. /// -/// A missing `operation` argument defaults to '[]='. -Never throwUnsupportedOperation(Object o, [String? operation]) { +/// The operation can be a string, or for more compact generated code, an index +/// into a small table of operation names. A missing `operation` or `verb` +/// argument defaults to index 0. +@pragma('dart2js:assumeDynamic') +Never throwUnsupportedOperation(Object o, [Object? operation, Object? verb]) { // Missing argument is defaulted manually. The calling convention for // top-level methods is that the call site provides the default values. Since - // the generated code omits the second argument, `undefined` is passed, which - // presents as Dart `null`. - operation ??= '[]='; + // the generated code omits the second or thrid argument, `undefined` is + // passed, which presents as Dart `null`. + operation ??= 0; + verb ??= 0; final wrapper = JS('', 'Error()'); throwExpressionWithWrapper( - _diagnoseUnsupportedOperation(o, operation), wrapper); + _diagnoseUnsupportedOperation(o, operation, verb), wrapper); } -Error _diagnoseUnsupportedOperation(Object o, String operation) { - String? verb; +@pragma('dart2js:never-inline') +Error _diagnoseUnsupportedOperation( + Object o, Object encodedOperation, Object encodedVerb) { + String operation; + String verb; + + if (encodedOperation is String) { + operation = encodedOperation; + } else { + final table = JS('', '#.split(";")', ArrayFlags.operationNames); + int tableLength = JS('JSUInt31', '#.length', table); + int index = JS('JSUInt31', '#', encodedOperation); + if (index > tableLength) { + // Verb is also encoded with the operation. + // Do math in JavaScript, we know there are no edge cases. + // Truncating divide: + encodedVerb = JS('', '(# / #) | 0', index, tableLength); + index = JS('', '# % #', index, tableLength); + } + // The index should be valid by construction. + operation = JS('', '#[#]', table, index); + } + + if (encodedVerb is String) { + verb = encodedVerb; + } else { + final table = JS('', '#.split(";")', ArrayFlags.verbs); + // The index should be valid by construction. + verb = JS('', '#[#]', table, encodedVerb); + } + String adjective = ''; - String article = 'a'; + String article = 'a '; String object; if (o is List) { object = 'list'; - // TODO(sra): Should we do more of this? - if (operation == 'add' || operation == 'addAll') verb ??= 'add to'; } else { object = 'ByteData'; - verb ??= "'$operation' on"; } - verb ??= 'modify'; + final flags = HArrayFlagsGet(o); - if (flags & ArrayFlags.unmodifiableCheck != 0) { + if (flags & ArrayFlags.constantCheck != 0) { + article = 'a '; + adjective = 'constant '; + } else if (flags & ArrayFlags.unmodifiableCheck != 0) { article = 'an '; adjective = 'unmodifiable '; - } else { - // No need to test for fixed-length, otherwise we would not be diagnosing - // the error. + } else if (flags & ArrayFlags.fixedLengthCheck != 0) { article = 'a '; adjective = 'fixed-length '; } - return UnsupportedError('Cannot $verb $article$adjective$object'); + + final prefix = "'$operation': "; + return UnsupportedError('${prefix}Cannot $verb $article$adjective$object'); } // This is used in open coded for-in loops on arrays. diff --git a/sdk/lib/_internal/js_runtime/lib/synced/array_flags.dart b/sdk/lib/_internal/js_runtime/lib/synced/array_flags.dart index d8ca41ea66f5..319b73729793 100644 --- a/sdk/lib/_internal/js_runtime/lib/synced/array_flags.dart +++ b/sdk/lib/_internal/js_runtime/lib/synced/array_flags.dart @@ -60,4 +60,64 @@ class ArrayFlags { /// Bit to check for constant JSArray. static const int constantCheck = 4; + + /// A List of operation names for HArrayFlagsCheck, encoded in a string of + /// names separated by semicolons. + /// + /// The optimizer may replace the string with an index into this + /// table. Generally this makes the generated code smaller. A name does not + /// need to be in this table. It makes sense for this table to include only + /// names that occur in more than one HArrayFlagsCheck, either as written in + /// js_runtime, or occuring multiple times via inlining. + static const operationNames = // + '[]=' // This is the most common operation so it comes first. + ';add' + ';removeWhere' + ';retainWhere' + ';removeRange' + ';setRange' + ';setInt8' + ';setInt16' + ';setInt32' + ';setUint8' + ';setUint16' + ';setUint32' + ';setFloat32' + ';setFloat64' + // + ; + + /// A list of 'verbs' for HArrayFlagsCheck, encoded as a string of phrases + /// separated by semicolons. The 'verb' fills a span of the error message, for + /// example, "remove from" in the message + /// + /// Cannot remove from an unmodifiable list. + /// ^^^^^^^^^^^ + /// + /// The optimizer may replace the string with an index into this + /// table. Generally this makes the program smaller. A 'verb' does not need to + /// be in this table. It makes sense to only have verbs that are used by + /// several calls to HArrayFlagsCheck, either as written in js_runtime, or + /// occuring multiple times via inlining. + static const verbs = // + 'modify' // This is the verb for '[]=' so it comes first. + ';remove from' + ';add to' + // + ; + + /// A view of [operationNames] as a map from the name to the index of the name + /// in the list. + static final Map operationNameToIndex = _invert(operationNames); + + /// A view of [verbs] as a map from the verb to the index of the verb in the + /// list. + static final Map verbToIndex = _invert(verbs); + + static Map _invert(String joinedStrings) { + return { + for (final entry in joinedStrings.split(';').asMap().entries) + entry.value: entry.key, + }; + } } diff --git a/tests/web/internal/array_flags_errors_test.dart b/tests/web/internal/array_flags_errors_test.dart new file mode 100644 index 000000000000..1afac8072c96 --- /dev/null +++ b/tests/web/internal/array_flags_errors_test.dart @@ -0,0 +1,34 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:_foreign_helper' show ArrayFlags, HArrayFlagsCheck; +import 'dart:typed_data'; + +import 'package:expect/expect.dart'; + +void rub(Object object, String message) { + try { + // Always fail, using non-standard operation name and verb. + HArrayFlagsCheck(object, ArrayFlags.unmodifiable, + ArrayFlags.fixedLengthCheck, 'rub', 'burnish'); + Expect.fail('HArrayFlagsCheck should always throw'); + } catch (e) { + Expect.equals(message, e.toString()); + } +} + +main() { + rub(const [], "Unsupported operation: 'rub': Cannot burnish a constant list"); + rub(List.unmodifiable([]), + "Unsupported operation: 'rub': Cannot burnish an unmodifiable list"); + rub(List.filled(0, 0), + "Unsupported operation: 'rub': Cannot burnish a fixed-length list"); + rub([], "Unsupported operation: 'rub': Cannot burnish a list"); + + rub(Uint8List(10).asUnmodifiableView(), + "Unsupported operation: 'rub': Cannot burnish an unmodifiable list"); + + rub(ByteData(10).asUnmodifiableView(), + "Unsupported operation: 'rub': Cannot burnish an unmodifiable ByteData"); +} diff --git a/tests/web/web.status b/tests/web/web.status index 55cc13631f80..c587463824c9 100644 --- a/tests/web/web.status +++ b/tests/web/web.status @@ -3,14 +3,12 @@ # BSD-style license that can be found in the LICENSE file. [ $compiler != dart2js ] +internal/array_flags_errors_test: SkipByDesign # test specific for the dart2js runtime internal/deferred_url_test: SkipByDesign # test specific for the dart2js runtime [ $compiler == dart2wasm ] (?!wasm)*: SkipByDesign -[ $compiler == dart2wasm && $runtime != d8 ] -wasm/source_map_simple_test: SkipByDesign - [ $compiler != dart2wasm ] wasm/*: SkipByDesign @@ -56,6 +54,9 @@ code_motion_exception_test: Skip # Requires unminified operator names. [ $compiler == dart2js && ($runtime == ff || $runtime == jsshell || $runtime == safari) ] code_motion_exception_test: Skip # Required V8 specific format of JavaScript errors. +[ $compiler == dart2wasm && $runtime != d8 ] +wasm/source_map_simple_test: SkipByDesign + [ $compiler == dartk && $runtime == vm ] new_from_env_test: SkipByDesign # dart2js only test unconditional_dartio_import_test: SkipByDesign # dart2js only test