From a5baf4e15b059ee82f897c5c98ea438cf08757c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Tue, 10 Sep 2024 09:03:21 +0000 Subject: [PATCH] [dart2wasm] Improve WasmListBase.{setRange,setAll} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - In `setRange`, use `array.copy` instruction when the iterable argument is another Wasm-array-backed list. (instead of only when the iterable and `this` are identical) - In `setRange`, add special case for `SubListIterable`. - In `setAll`, use unchecked reads from `iterable` when it's a Wasm-array-backed list. Also fix various error checking in `setAll` and `setRange`. `setAll` and `setRange` tests updated to test handling of different types of iterable arguments. CoreLibraryReviewExempt: adds internal method Change-Id: Ib3ed566018e929950eac4d1f3d41dcd406003f91 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/383860 Reviewed-by: Martin Kustermann Reviewed-by: Lasse Nielsen Commit-Queue: Ömer Ağacan --- sdk/lib/_internal/wasm/lib/list.dart | 116 +++++++++++++----- sdk/lib/internal/iterable.dart | 8 ++ tests/corelib/list_set_all_test.dart | 113 +++++++++-------- tests/corelib/list_set_range_test.dart | 162 ++++++++++++++++--------- 4 files changed, 258 insertions(+), 141 deletions(-) diff --git a/sdk/lib/_internal/wasm/lib/list.dart b/sdk/lib/_internal/wasm/lib/list.dart index 5a506c2c5ad..4d429dafce0 100644 --- a/sdk/lib/_internal/wasm/lib/list.dart +++ b/sdk/lib/_internal/wasm/lib/list.dart @@ -67,54 +67,116 @@ abstract class _ModifiableList extends WasmListBase { : super._withData(length, data); @pragma('wasm:prefer-inline') + @override void operator []=(int index, E value) { indexCheckWithName(index, _length, "[]="); _data[index] = value; } - // List interface. + @override void setRange(int start, int end, Iterable iterable, [int skipCount = 0]) { RangeError.checkValidRange(start, end, this.length); int length = end - start; if (length == 0) return; RangeError.checkNotNegative(skipCount, "skipCount"); - if (identical(this, iterable)) { - _data.copy(start, _data, skipCount, length); - } else if (iterable is List) { - Lists.copy(iterable, skipCount, this, start, length); - } else { - Iterator it = iterable.iterator; - while (skipCount > 0) { - if (!it.moveNext()) return; - skipCount--; + + // Look through `SubListIterable`s while still testing for the fast case as + // first thing. + while (true) { + if (iterable is WasmListBase) { + final iterableWasmList = unsafeCast(iterable); + if (skipCount + length > iterableWasmList.length) { + throw IterableElementError.tooFew(); + } + _data.copy(start, iterableWasmList._data, skipCount, length); + return; } - for (int i = start; i < end; i++) { - if (!it.moveNext()) return; - _data[i] = it.current; + + if (iterable is List) { + final iterableList = unsafeCast>(iterable); + for (int i = skipCount, j = start; i < skipCount + length; i++, j++) { + _data[j] = iterableList[i]; + } + return; } + + if (iterable is SubListIterable) { + final listIterable = unsafeCast>(iterable); + var sourceLength = listIterable.length; + if (sourceLength - skipCount < length) { + throw IterableElementError.tooFew(); + } + iterable = SubListIterable.iterableOf(listIterable); + skipCount += SubListIterable.startOf(listIterable); + continue; + } + + break; + } + + Iterator it = iterable.iterator; + while (skipCount > 0) { + if (!it.moveNext()) throw IterableElementError.tooFew(); + skipCount--; + } + for (int i = start; i < end; i++) { + if (!it.moveNext()) throw IterableElementError.tooFew(); + _data[i] = it.current; } } + @override void setAll(int index, Iterable iterable) { - if (index < 0 || index > this.length) { - throw RangeError.range(index, 0, this.length, "index"); + final length = this.length; + + // index < 0 || index > length + if (index.gtU(length)) { + throw RangeError.range(index, 0, length, "index"); } - List iterableAsList; - if (identical(this, iterable)) { - iterableAsList = this; - } else if (iterable is List) { - iterableAsList = iterable; - } else { - for (var value in iterable) { - this[index++] = value; + + if (iterable is WasmListBase) { + final iterableWasmList = unsafeCast(iterable); + final elementCount = iterableWasmList.length; + + // Elements to copy = min(length - index, elementCount). + int copyCount = length - index; + if (copyCount > elementCount) { + copyCount = elementCount; } + + _data.copy(index, iterableWasmList._data, 0, copyCount); + + if (elementCount > copyCount) { + throw IndexError.withLength(length, length); + } + return; } - int length = iterableAsList.length; - if (index + length > this.length) { - throw RangeError.range(index + length, 0, this.length); + + if (iterable is List) { + final iterableList = unsafeCast>(iterable); + final elementCount = iterableList.length; + + // Elements to copy = min(length - index, elementCount). + int copyCount = length - index; + if (copyCount > elementCount) { + copyCount = elementCount; + } + + for (int i = 0, j = index; i < copyCount; i++, j++) { + _data[j] = iterableList[i]; + } + + if (elementCount > copyCount) { + throw IndexError.withLength(length, length); + } + + return; + } + + for (var value in iterable) { + this[index++] = value; } - Lists.copy(iterableAsList, 0, this, index, length); } @override diff --git a/sdk/lib/internal/iterable.dart b/sdk/lib/internal/iterable.dart index b6f58166bbe..01f4ea7a58a 100644 --- a/sdk/lib/internal/iterable.dart +++ b/sdk/lib/internal/iterable.dart @@ -238,6 +238,14 @@ base class SubListIterable extends ListIterable { /** If null, represents the length of the iterable. */ final int? _endOrLength; + /// Returns `_iterable` for for internal code. + static Iterable iterableOf(SubListIterable subListIterable) => + subListIterable._iterable; + + /// Returns `_start` for for internal code. + static int startOf(SubListIterable subListIterable) => + subListIterable._start; + SubListIterable(this._iterable, this._start, this._endOrLength) { RangeError.checkNotNegative(_start, "start"); int? endOrLength = _endOrLength; diff --git a/tests/corelib/list_set_all_test.dart b/tests/corelib/list_set_all_test.dart index 85f208100bc..bdb608b8392 100644 --- a/tests/corelib/list_set_all_test.dart +++ b/tests/corelib/list_set_all_test.dart @@ -2,10 +2,11 @@ // 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 "package:expect/expect.dart"; import "dart:collection"; -test(List list, int index, Iterable iterable) { +import "package:expect/expect.dart"; + +void test(List list, int index, Iterable iterable) { List copy = list.toList(); list.setAll(index, iterable); Expect.equals(copy.length, list.length); @@ -22,7 +23,7 @@ test(List list, int index, Iterable iterable) { } class MyList extends ListBase { - List list; + final List list; MyList(this.list); get length => list.length; set length(value) { @@ -37,61 +38,55 @@ class MyList extends ListBase { toString() => list.toString(); } -main() { - test([1, 2, 3], 0, [4, 5]); - test([1, 2, 3], 1, [4, 5]); - test([1, 2, 3], 2, [4]); - test([1, 2, 3], 3, []); - test([1, 2, 3], 0, [4, 5].map((x) => x)); - test([1, 2, 3], 1, [4, 5].map((x) => x)); - test([1, 2, 3], 2, [4].map((x) => x)); - test([1, 2, 3], 3, [].map((x) => x)); - test([1, 2, 3], 0, const [4, 5]); - test([1, 2, 3], 1, const [4, 5]); - test([1, 2, 3], 2, const [4]); - test([1, 2, 3], 3, const []); - test([1, 2, 3], 0, new Iterable.generate(2, (x) => x + 4)); - test([1, 2, 3], 1, new Iterable.generate(2, (x) => x + 4)); - test([1, 2, 3], 2, new Iterable.generate(1, (x) => x + 4)); - test([1, 2, 3], 3, new Iterable.generate(0, (x) => x + 4)); - test([1, 2, 3].toList(growable: false), 0, [4, 5]); - test([1, 2, 3].toList(growable: false), 1, [4, 5]); - test([1, 2, 3].toList(growable: false), 2, [4]); - test([1, 2, 3].toList(growable: false), 3, []); - test([1, 2, 3].toList(growable: false), 0, [4, 5].map((x) => x)); - test([1, 2, 3].toList(growable: false), 1, [4, 5].map((x) => x)); - test([1, 2, 3].toList(growable: false), 2, [4].map((x) => x)); - test([1, 2, 3].toList(growable: false), 3, [].map((x) => x)); - test([1, 2, 3].toList(growable: false), 0, const [4, 5]); - test([1, 2, 3].toList(growable: false), 1, const [4, 5]); - test([1, 2, 3].toList(growable: false), 2, const [4]); - test([1, 2, 3].toList(growable: false), 3, const []); - test([1, 2, 3].toList(growable: false), 0, - new Iterable.generate(2, (x) => x + 4)); - test([1, 2, 3].toList(growable: false), 1, - new Iterable.generate(2, (x) => x + 4)); - test([1, 2, 3].toList(growable: false), 2, - new Iterable.generate(1, (x) => x + 4)); - test([1, 2, 3].toList(growable: false), 3, - new Iterable.generate(0, (x) => x + 4)); - test(new MyList([1, 2, 3]), 0, [4, 5]); - test(new MyList([1, 2, 3]), 1, [4, 5]); - test(new MyList([1, 2, 3]), 2, [4]); - test(new MyList([1, 2, 3]), 3, []); - test(new MyList([1, 2, 3]), 0, [4, 5].map((x) => x)); - test(new MyList([1, 2, 3]), 1, [4, 5].map((x) => x)); - test(new MyList([1, 2, 3]), 2, [4].map((x) => x)); - test(new MyList([1, 2, 3]), 3, [].map((x) => x)); +void main() { + for (var makeIterable in iterableMakers) { + test([1, 2, 3], 0, makeIterable([4, 5])); + test([1, 2, 3], 1, makeIterable([4, 5])); + test([1, 2, 3], 2, makeIterable([4])); + test([1, 2, 3], 3, makeIterable([])); + test([1, 2, 3], 0, makeIterable(const [4, 5])); + test([1, 2, 3], 1, makeIterable(const [4, 5])); + test([1, 2, 3], 2, makeIterable(const [4])); + test([1, 2, 3], 3, makeIterable(const [])); + test([1, 2, 3].toList(growable: false), 0, makeIterable([4, 5])); + test([1, 2, 3].toList(growable: false), 1, makeIterable([4, 5])); + test([1, 2, 3].toList(growable: false), 2, makeIterable([4])); + test([1, 2, 3].toList(growable: false), 3, makeIterable([])); + test([1, 2, 3].toList(growable: false), 0, makeIterable(const [4, 5])); + test([1, 2, 3].toList(growable: false), 1, makeIterable(const [4, 5])); + test([1, 2, 3].toList(growable: false), 2, makeIterable(const [4])); + test([1, 2, 3].toList(growable: false), 3, makeIterable(const [])); + test(MyList([1, 2, 3]), 0, makeIterable([4, 5])); + test(MyList([1, 2, 3]), 1, makeIterable([4, 5])); + test(MyList([1, 2, 3]), 2, makeIterable([4])); + test(MyList([1, 2, 3]), 3, makeIterable([])); - Expect.throwsRangeError(() => test([1, 2, 3], -1, [4, 5])); - Expect.throwsRangeError( - () => test([1, 2, 3].toList(growable: false), -1, [4, 5])); - Expect.throwsRangeError(() => test([1, 2, 3], 1, [4, 5, 6])); - Expect.throwsRangeError( - () => test([1, 2, 3].toList(growable: false), 1, [4, 5, 6])); - Expect.throwsRangeError(() => test(new MyList([1, 2, 3]), -1, [4, 5])); - Expect.throwsRangeError(() => test(new MyList([1, 2, 3]), 1, [4, 5, 6])); - Expect.throwsUnsupportedError(() => test(const [1, 2, 3], 0, [4, 5])); - Expect.throwsUnsupportedError(() => test(const [1, 2, 3], -1, [4, 5])); - Expect.throwsUnsupportedError(() => test(const [1, 2, 3], 1, [4, 5, 6])); + Expect.throwsRangeError(() => test([1, 2, 3], -1, makeIterable([4, 5]))); + Expect.throwsRangeError(() => + test([1, 2, 3].toList(growable: false), -1, makeIterable([4, 5]))); + Expect.throwsRangeError(() => test([1, 2, 3], 1, makeIterable([4, 5, 6]))); + Expect.throwsRangeError(() => + test([1, 2, 3].toList(growable: false), 1, makeIterable([4, 5, 6]))); + Expect.throwsRangeError( + () => test(MyList([1, 2, 3]), -1, makeIterable([4, 5]))); + Expect.throwsRangeError( + () => test(MyList([1, 2, 3]), 1, makeIterable([4, 5, 6]))); + Expect.throwsUnsupportedError( + () => test(const [1, 2, 3], 0, makeIterable([4, 5]))); + Expect.throwsUnsupportedError( + () => test(const [1, 2, 3], -1, makeIterable([4, 5]))); + Expect.throwsUnsupportedError( + () => test(const [1, 2, 3], 1, makeIterable([4, 5, 6]))); + } } + +// `setAll` implementations can have type tests and special cases to handle +// different types of iterables differently, so we test with a few different +// types of iterables. +List Function(List)> iterableMakers = [ + (list) => list, + MyList.new, + (list) => list.where((x) => true), + (list) => list.map((x) => x), + (list) => list.getRange(0, list.length), +]; diff --git a/tests/corelib/list_set_range_test.dart b/tests/corelib/list_set_range_test.dart index fcc56d6c3ef..1cf989068d0 100644 --- a/tests/corelib/list_set_range_test.dart +++ b/tests/corelib/list_set_range_test.dart @@ -2,84 +2,136 @@ // 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:collection"; + import "package:expect/expect.dart"; -main() { - var list = []; - list.setRange(0, 0, const []); - list.setRange(0, 0, []); - list.setRange(0, 0, const [], 1); - list.setRange(0, 0, [], 1); - Expect.equals(0, list.length); - Expect.throwsRangeError(() => list.setRange(0, 1, [])); - Expect.throwsRangeError(() => list.setRange(0, 1, [], 1)); - Expect.throwsRangeError(() => list.setRange(0, 1, [1], 0)); +void main() { + for (var makeIterable in iterableMakers) { + var list = []; + list.setRange(0, 0, makeIterable(const [])); + list.setRange(0, 0, makeIterable([])); + list.setRange(0, 0, makeIterable(const []), 1); + list.setRange(0, 0, makeIterable([]), 1); + Expect.listEquals([], list); + Expect.throwsRangeError(() => list.setRange(0, 1, [])); + Expect.throwsRangeError(() => list.setRange(0, 1, [], 1)); + Expect.throwsRangeError(() => list.setRange(0, 1, [1], 0)); - list.add(1); - list.setRange(0, 0, [], 0); - Expect.equals(1, list.length); - Expect.equals(1, list[0]); - list.setRange(0, 0, const [], 0); - Expect.equals(1, list.length); - Expect.equals(1, list[0]); + list = [1]; + list.setRange(0, 0, makeIterable([]), 0); + Expect.listEquals([1], list); + list.setRange(0, 0, makeIterable(const []), 0); + Expect.listEquals([1], list); - Expect.throwsRangeError(() => list.setRange(0, 2, [1, 2])); - Expect.equals(1, list.length); - Expect.equals(1, list[0]); + Expect.throwsRangeError(() => list.setRange(0, 2, [1, 2])); + Expect.listEquals([1], list); - Expect.throwsStateError(() => list.setRange(0, 1, [1, 2], 2)); - Expect.equals(1, list.length); - Expect.equals(1, list[0]); + Expect.throwsStateError(() => list.setRange(0, 1, [1, 2], 2)); + Expect.listEquals([1], list); - list.setRange(0, 1, [2], 0); - Expect.equals(1, list.length); - Expect.equals(2, list[0]); + Expect.throwsStateError(() => list.setRange(0, 1, list, 2)); + Expect.listEquals([1], list); - list.setRange(0, 1, const [3], 0); - Expect.equals(1, list.length); - Expect.equals(3, list[0]); + list.setRange(0, 1, makeIterable([2]), 0); + Expect.listEquals([2], list); - list.addAll([4, 5, 6]); - Expect.equals(4, list.length); - list.setRange(0, 4, [1, 2, 3, 4]); - Expect.listEquals([1, 2, 3, 4], list); + list.setRange(0, 1, makeIterable(const [3]), 0); + Expect.listEquals([3], list); - list.setRange(2, 4, [5, 6, 7, 8]); - Expect.listEquals([1, 2, 5, 6], list); + list = [3, 4, 5, 6]; + list.setRange(0, 4, makeIterable([1, 2, 3, 4])); + Expect.listEquals([1, 2, 3, 4], list); - Expect.throwsRangeError(() => list.setRange(4, 5, [5, 6, 7, 8])); - Expect.listEquals([1, 2, 5, 6], list); + list.setRange(2, 4, makeIterable([5, 6, 7, 8])); + Expect.listEquals([1, 2, 5, 6], list); - list.setRange(1, 3, [9, 10, 11, 12]); - Expect.listEquals([1, 9, 10, 6], list); + Expect.throwsRangeError( + () => list.setRange(4, 5, makeIterable([5, 6, 7, 8]))); + Expect.listEquals([1, 2, 5, 6], list); + + list.setRange(1, 3, makeIterable([9, 10, 11, 12])); + Expect.listEquals([1, 9, 10, 6], list); + } testNegativeIndices(); testNonExtendableList(); + + testNotEnoughElements(); } void testNegativeIndices() { - var list = [1, 2]; - Expect.throwsRangeError(() => list.setRange(-1, 1, [1])); - Expect.throwsArgumentError(() => list.setRange(0, 1, [1], -1)); + for (var makeIterable in iterableMakers) { + var list = [1, 2]; + Expect.throwsRangeError(() => list.setRange(-1, 1, makeIterable([1]))); + Expect.throwsArgumentError( + () => list.setRange(0, 1, makeIterable([1]), -1)); - Expect.throwsRangeError(() => list.setRange(2, 1, [1])); + Expect.throwsRangeError(() => list.setRange(2, 1, makeIterable([1]))); - Expect.throwsArgumentError(() => list.setRange(-1, -2, [1], -1)); - Expect.listEquals([1, 2], list); + Expect.throwsArgumentError( + () => list.setRange(-1, -2, makeIterable([1]), -1)); + Expect.listEquals([1, 2], list); - Expect.throwsRangeError(() => list.setRange(-1, -1, [1])); - Expect.listEquals([1, 2], list); + Expect.throwsRangeError(() => list.setRange(-1, -1, makeIterable([1]))); + Expect.listEquals([1, 2], list); - // The skipCount is only used if the length is not 0. - list.setRange(0, 0, [1], -1); - Expect.listEquals([1, 2], list); + // The skipCount is only used if the length is not 0. + list.setRange(0, 0, makeIterable([1]), -1); + Expect.listEquals([1, 2], list); + } } void testNonExtendableList() { - var list = new List.filled(6, null); - Expect.listEquals([null, null, null, null, null, null], list); - list.setRange(0, 3, [1, 2, 3, 4]); - list.setRange(3, 6, [1, 2, 3, 4]); - Expect.listEquals([1, 2, 3, 1, 2, 3], list); + for (var makeIterable in iterableMakers) { + var list = List.filled(6, null); + Expect.listEquals([null, null, null, null, null, null], list); + list.setRange(0, 3, makeIterable([1, 2, 3, 4])); + list.setRange(3, 6, makeIterable([1, 2, 3, 4])); + Expect.listEquals([1, 2, 3, 1, 2, 3], list); + } } + +// Test errors when there aren't enough elements in the source iterable. +void testNotEnoughElements() { + // Check errors when the source doesn't have enough elements after skipping + // `skipCount` elements. + for (var makeIterable in iterableMakers) { + List list = [1, 2, 3, 4, 5]; + Expect.throws(() => list.setRange(0, 5, makeIterable([9, 9, 9]))); + } + + // Check errors when the source doesn't have `skipCount` elements. + for (var makeIterable in iterableMakers) { + List list = [1, 2, 3, 4, 5]; + Expect.throws(() => list.setRange(0, 5, makeIterable([9, 9, 9]), 5)); + } +} + +class MyList extends ListBase { + final List list; + MyList(this.list); + get length => list.length; + set length(value) { + list.length = value; + } + + operator [](index) => list[index]; + operator []=(index, val) { + list[index] = val; + } + + toString() => list.toString(); +} + +// `setRange` implementations can have type tests and special cases to handle +// different types of iterables differently, so we test with a few different +// types of iterables. +List Function(List)> iterableMakers = [ + (list) => list, + MyList.new, + (list) => list.where((x) => true), + (list) => list.map((x) => x), + (list) => list.getRange(0, list.length), +];