[M] Change in dart/sdk[main]: [vm] Ensure we copy shallow (but not deeply) immutable lists
0 views
Skip to first unread message
Martin Kustermann (Gerrit)
Feb 7, 2023, 9:46:56 AM2/7/23
to Ryan Macnak, Alexander Aprelev, rev...@dartlang.org, vm-...@dartlang.org
Attention is currently required from: Alexander Aprelev, Ryan Macnak.
Martin Kustermann would like Ryan Macnak and Alexander Aprelev to review this change.
[vm] Ensure we copy shallow (but not deeply) immutable lists
The object graph copy code assumed kImmutableCid is used only for
list constants but it turns out our List.immutable([]) implementation
isn't using a wrapper-view, but instead a fixed-length immutable list as
with constants:
factory List.unmodifiable(Iterable elements) {
final result = new List<E>.from(elements, growable: false);
return makeFixedListUnmodifiable(result);
}
The list constants should already be handled via canonical bit.
Issue https://github.com/dart-lang/sdk/issues/51302
TEST=vm/dart{,_2}/isolates/regress_51302_test
Change-Id: I5924084ff004dc52bf675897455453d728484d1d
---
A runtime/tests/vm/dart/isolates/regress_51302_test.dart
A runtime/tests/vm/dart_2/isolates/regress_51302_test.dart
M runtime/vm/object_graph_copy.cc
3 files changed, 91 insertions(+), 5 deletions(-)
diff --git a/runtime/tests/vm/dart/isolates/regress_51302_test.dart b/runtime/tests/vm/dart/isolates/regress_51302_test.dart
new file mode 100644
index 0000000..5a6c561
--- /dev/null
+++ b/runtime/tests/vm/dart/isolates/regress_51302_test.dart
@@ -0,0 +1,27 @@
+// Copyright (c) 2023, 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:isolate';
+
+import 'package:expect/expect.dart';
+
+main() async {
+ final original = List<C>.unmodifiable([C()]);
+ final copy = await sendReceive(original);
+ Expect.notIdentical(original, copy);
+
+ original[0].field = 1;
+ Expect.equals(1, original[0].field);
+ Expect.equals(0, copy[0].field);
+}
+
+Future<T> sendReceive<T>(T arg) async {
+ final rp = ReceivePort();
+ rp.sendPort.send(arg);
+ return (await rp.first) as T;
+}
+
+class C {
+ int field = 0;
+}
diff --git a/runtime/tests/vm/dart_2/isolates/regress_51302_test.dart b/runtime/tests/vm/dart_2/isolates/regress_51302_test.dart
new file mode 100644
index 0000000..88b5642
--- /dev/null
+++ b/runtime/tests/vm/dart_2/isolates/regress_51302_test.dart
@@ -0,0 +1,29 @@
+// Copyright (c) 2023, 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.
+
+// @dart=2.9
+
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+main() async {
+ final original = List<C>.unmodifiable([C()]);
+ final copy = await sendReceive(original);
+ Expect.notIdentical(original, copy);
+
+ original[0].field = 1;
+ Expect.equals(1, original[0].field);
+ Expect.equals(0, copy[0].field);
+}
+
+Future<T> sendReceive<T>(T arg) async {
+ final rp = ReceivePort();
+ rp.sendPort.send(arg);
+ return (await rp.first) as T;
+}
+
+class C {
+ int field = 0;
+}
diff --git a/runtime/vm/object_graph_copy.cc b/runtime/vm/object_graph_copy.cc
index 7f42d12..42cea0f 100644
--- a/runtime/vm/object_graph_copy.cc
+++ b/runtime/vm/object_graph_copy.cc
@@ -156,7 +156,6 @@
if (cid == kExternalOneByteStringCid) return true;
if (cid == kExternalTwoByteStringCid) return true;
if (cid == kMintCid) return true;
- if (cid == kImmutableArrayCid) return true;
if (cid == kNeverCid) return true;
if (cid == kSentinelCid) return true;
if (cid == kStackTraceCid) return true;
@@ -209,7 +208,6 @@
// These are shared and use identity hash codes. If they are used as a key in
// a map or a value in a set, they will already have the identity hash code
// set.
- if (cid == kImmutableArrayCid) return false;
if (cid == kRegExpCid) return false;
if (cid == kInt32x4Cid) return false;
@@ -270,11 +268,10 @@
void UpdateLengthField(intptr_t cid, ObjectPtr from, ObjectPtr to) {
// We share these objects - never copy them.
ASSERT(!IsStringClassId(cid));
- ASSERT(cid != kImmutableArrayCid);
// We update any in-heap variable sized object with the length to keep the
// length and the size in the object header in-sync for the GC.
- if (cid == kArrayCid) {
+ if (cid == kArrayCid || cid == kImmutableArrayCid) {
static_cast<UntaggedArray*>(to.untag())->length_ =
static_cast<UntaggedArray*>(from.untag())->length_;
} else if (cid == kContextCid) {
@@ -1203,7 +1200,8 @@
UpdateLengthField(cid, from.ptr(), to_.ptr());
slow_forward_map_.Insert(from, to_, size);
ObjectPtr to = to_.ptr();
- if (cid == kArrayCid && !Heap::IsAllocatableInNewSpace(size)) {
+ if ((cid == kArrayCid || cid == kImmutableArrayCid) &&
+ !Heap::IsAllocatableInNewSpace(size)) {
to.untag()->SetCardRememberedBitUnsynchronized();
}
if (IsExternalTypedDataClassId(cid)) {
@@ -1318,6 +1316,13 @@
COPY_TO(Set)
#undef COPY_TO
+ case ImmutableArray::kClassId: {
+ typename Types::Array casted_from = Types::CastArray(from);
+ typename Types::Array casted_to = Types::CastArray(to);
+ CopyArray(casted_from, casted_to);
+ return;
+ }
+
#define COPY_TO(clazz) case kTypedData##clazz##Cid:
CLASS_LIST_TYPED_DATA(COPY_TO) {
To view, visit change 281421. To unsubscribe, or for help writing mail filters, visit settings.
Martin Kustermann (Gerrit)
Feb 7, 2023, 9:46:57 AM2/7/23
to rev...@dartlang.org, vm-...@dartlang.org, Ryan Macnak, Alexander Aprelev, CBuild, Commit Queue
Attention is currently required from: Alexander Aprelev, Ryan Macnak.
Alexander Aprelev (Gerrit)
Feb 7, 2023, 10:08:02 AM2/7/23
to Martin Kustermann, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, Ryan Macnak, CBuild, Commit Queue
Attention is currently required from: Martin Kustermann, Ryan Macnak.
Patch set 2:Code-Review +1
Ryan Macnak (Gerrit)
Feb 7, 2023, 5:24:25 PM2/7/23
to Martin Kustermann, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, CBuild, Commit Queue
Attention is currently required from: Martin Kustermann.
Patch set 2:Code-Review +1
To view, visit change 281421. To unsubscribe, or for help writing mail filters, visit settings.
Martin Kustermann (Gerrit)
Feb 8, 2023, 3:58:27 AM2/8/23
to rev...@dartlang.org, vm-...@dartlang.org, Ryan Macnak, Alexander Aprelev, CBuild, Commit Queue
Attention is currently required from: Martin Kustermann.
Patch set 2:Commit-Queue +2
Commit Queue (Gerrit)
Feb 8, 2023, 3:58:45 AM2/8/23
to Martin Kustermann, rev...@dartlang.org, vm-...@dartlang.org, Ryan Macnak, Alexander Aprelev, CBuild
Commit Queue submitted this change.
Approvals:
Alexander Aprelev: Looks good to me, approved
Ryan Macnak: Looks good to me, approved
Martin Kustermann: Commit
[vm] Ensure we copy shallow (but not deeply) immutable lists
The object graph copy code assumed kImmutableCid is used only for
list constants but it turns out our List.immutable([]) implementation
isn't using a wrapper-view, but instead a fixed-length immutable list as
with constants:
factory List.unmodifiable(Iterable elements) {
final result = new List<E>.from(elements, growable: false);
return makeFixedListUnmodifiable(result);
}
The list constants should already be handled via canonical bit.
Issue https://github.com/dart-lang/sdk/issues/51302
TEST=vm/dart{,_2}/isolates/regress_51302_test
Change-Id: I5924084ff004dc52bf675897455453d728484d1d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/281421
Reviewed-by: Alexander Aprelev <a...@google.com>
Reviewed-by: Ryan Macnak <rma...@google.com>
Commit-Queue: Martin Kustermann <kuste...@google.com>
---
A runtime/tests/vm/dart/isolates/regress_51302_test.dart
A runtime/tests/vm/dart_2/isolates/regress_51302_test.dart
M runtime/vm/object_graph_copy.cc
3 files changed, 95 insertions(+), 5 deletions(-)
index 5250dcb..c21dc33 100644To view, visit change 281421. To unsubscribe, or for help writing mail filters, visit settings.
Martin Kustermann (Gerrit)
Feb 8, 2023, 3:58:52 AM2/8/23
to rev...@dartlang.org, vm-...@dartlang.org, Commit Queue, Ryan Macnak, Alexander Aprelev, CBuild
Patch set 2:Commit-Queue +2
1 comment:
Patchset:
Thanks, Ryan & Alex!
To view, visit change 281421. To unsubscribe, or for help writing mail filters, visit settings.
CBuild (Gerrit)
Feb 8, 2023, 5:18:25 AM2/8/23
to Commit Queue, Martin Kustermann, rev...@dartlang.org, vm-...@dartlang.org, Ryan Macnak, Alexander Aprelev
go/dart-cbuild result: SUCCESS
Details: https://goto.google.com/dart-cbuild/find/63b5f5d4b1c53ce5818fa2648492869bdaf104bf
Reply all
Reply to author
Forward
0 new messages