[M] Change in dart/sdk[main]: [vm] Allow sharing of deeply lists across isolates

Martin Kustermann (Gerrit)

unread,

Feb 7, 2023, 9:48:43 AM2/7/23

to Ryan Macnak, Slava Egorov, rev...@dartlang.org, vm-...@dartlang.org

Attention is currently required from: Ryan Macnak, Slava Egorov.

Martin Kustermann would like Ryan Macnak and Slava Egorov to review this change.

[vm] Allow sharing of deeply lists across isolates

We support allocating deeply immutable typed data in the Dart C API
already.
=> This CL adds a test for that.

Furthermore we hook into the already O(n) construction of
`List.unmodifiable(<list>)` and set the objects immutable bit
if all elements of `<list>` are also immutable.
=> This allows then sharing of such lists when sent across isolates.

We do this eagerly to

  - avoid introducing complexity to the deep immutability check in
    transitive copy algorithm

  - avoids re-checking for the subgraph on every send (similar to
    canonicalized bit for constants, we can simply rely on the
    immutable bit)

If we ever optimized `List.unmodifiable()` (which always performs
runtime call atm) we could fold this checking into the place that copies
list elements from `<list>` to the newly allocated list.

Issue https://github.com/dart-lang/sdk/issues/51302

TEST=vm/dart{,_2}/isolates/fast_object_copy2_test

Change-Id: Ie275e65036a4c53d992804972aae741a9f5ed6dd
---
M runtime/lib/growable_array.cc
M runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart
M runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart
M runtime/vm/object_graph_copy.cc
M runtime/vm/object_graph_copy.h
5 files changed, 150 insertions(+), 0 deletions(-)

diff --git a/runtime/lib/growable_array.cc b/runtime/lib/growable_array.cc
index 1f8bd54..ff51131 100644
--- a/runtime/lib/growable_array.cc
+++ b/runtime/lib/growable_array.cc
@@ -8,6 +8,7 @@
 #include "vm/exceptions.h"
 #include "vm/native_entry.h"
 #include "vm/object.h"
+#include "vm/object_graph_copy.h"
 
 namespace dart {
 
@@ -89,6 +90,17 @@
 DEFINE_NATIVE_ENTRY(Internal_makeFixedListUnmodifiable, 0, 1) {
   GET_NON_NULL_NATIVE_ARGUMENT(Array, array, arguments->NativeArgAt(0));
   array.MakeImmutable();
+  const intptr_t len = array.Length();
+  bool is_deeply_immutable = true;
+  for (intptr_t i = 0; i < len; ++i) {
+    if (!CanShareObjectAcrossIsolates(array.At(i))) {
+      is_deeply_immutable = false;
+      break;
+    }
+  }
+  if (is_deeply_immutable) {
+    array.SetImmutable();
+  }
   return array.ptr();
 }
 
diff --git a/runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart b/runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart
index 53a55a7..d7a143e 100644
--- a/runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart
+++ b/runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart
@@ -8,10 +8,12 @@
 // VMOptions=--enable-fast-object-copy --gc-on-foc-slow-path --force-evacuation
 
 import 'dart:async';
+import 'dart:ffi';
 import 'dart:isolate';
 import 'dart:typed_data';
 
 import 'package:expect/expect.dart';
+import 'package:ffi/ffi.dart';
 
 import 'fast_object_copy_test.dart'
     show UserObject, SendReceiveTestBase, notAllocatableInTLAB;
@@ -108,6 +110,8 @@
     await testSharable();
     await testSharable2();
     await testCopyableClosures();
+    await testSharableTypedData();
+    await testUnmodifiableList();
   }
 
   Future testSharable() async {
@@ -155,8 +159,61 @@
       Expect.equals(copyableClosures[i].runtimeType, copy2[i].runtimeType);
     }
   }
+
+  Future testSharableTypedData() async {
+    print('testSharableTypedData');
+    const int Dart_TypedData_kUint8 = 2;
+    const int count = 10;
+    final bytes = malloc.allocate<Uint8>(count);
+    final td = createUnmodifiableTypedData(
+        Dart_TypedData_kUint8, bytes, count, nullptr, 0, nullptr);
+    Expect.equals(count, td.length);
+
+    {
+      final copiedTd = await sendReceive(td);
+      Expect.identical(td, copiedTd);
+    }
+
+    malloc.free(bytes);
+  }
+
+  Future testUnmodifiableList() async {
+    print('testUnmodifiableList');
+
+    // Sharable.
+    for (final sharable in [
+      1,
+      const Object(),
+      'foo',
+      List.unmodifiable([const Object()])
+    ]) {
+      final list = List.unmodifiable([sharable]);
+      final listCopy = await sendReceive(list);
+      Expect.identical(list, listCopy);
+    }
+
+    // Non-Sharable.
+    for (final nonSharable in [
+      Object(),
+      [],
+      {},
+      List.unmodifiable([Object()])
+    ]) {
+      final list = List.unmodifiable([nonSharable]);
+      final listCopy = await sendReceive(list);
+      Expect.notIdentical(list, listCopy);
+      Expect.notIdentical(list[0], listCopy[0]);
+    }
+  }
 }
 
 main() async {
   await SendReceiveTest().run();
 }
+
+@Native<
+        Handle Function(
+            Int, Pointer<Uint8>, IntPtr, Pointer<Void>, IntPtr, Pointer<Void>)>(
+    symbol: "Dart_NewUnmodifiableExternalTypedDataWithFinalizer")
+external Uint8List createUnmodifiableTypedData(int type, Pointer<Uint8> data,
+    int length, Pointer<Void> peer, int externalSize, Pointer<Void> callback);
diff --git a/runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart b/runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart
index 673481c..6d41f1a 100644
--- a/runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart
+++ b/runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart
@@ -10,10 +10,12 @@
 // VMOptions=--enable-fast-object-copy --gc-on-foc-slow-path --force-evacuation
 
 import 'dart:async';
+import 'dart:ffi';
 import 'dart:isolate';
 import 'dart:typed_data';
 
 import 'package:expect/expect.dart';
+import 'package:ffi/ffi.dart';
 
 import 'fast_object_copy_test.dart'
     show UserObject, SendReceiveTestBase, notAllocatableInTLAB;
@@ -109,6 +111,7 @@
     await testSharable();
     await testSharable2();
     await testCopyableClosures();
+    await testUnmodifiableList();
   }
 
   Future testSharable() async {
@@ -156,6 +159,35 @@
       Expect.equals(copyableClosures[i].runtimeType, copy2[i].runtimeType);
     }
   }
+
+  Future testUnmodifiableList() async {
+    print('testUnmodifiableList');
+
+    // Sharable.
+    for (final sharable in [
+      1,
+      const Object(),
+      'foo',
+      List.unmodifiable([const Object()])
+    ]) {
+      final list = List.unmodifiable([sharable]);
+      final listCopy = await sendReceive(list);
+      Expect.identical(list, listCopy);
+    }
+
+    // Non-Sharable.
+    for (final nonSharable in [
+      Object(),
+      [],
+      {},
+      List.unmodifiable([Object()])
+    ]) {
+      final list = List.unmodifiable([nonSharable]);
+      final listCopy = await sendReceive(list);
+      Expect.notIdentical(list, listCopy);
+      Expect.notIdentical(list[0], listCopy[0]);
+    }
+  }
 }
 
 main() async {
diff --git a/runtime/vm/object_graph_copy.cc b/runtime/vm/object_graph_copy.cc
index 42cea0f..f9092ab 100644
--- a/runtime/vm/object_graph_copy.cc
+++ b/runtime/vm/object_graph_copy.cc
@@ -179,11 +179,21 @@
         ->typed_data()
         ->untag()
         ->IsImmutable();
+  } else {
+    if ((tags & UntaggedObject::ImmutableBit::mask_in_place()) != 0) {
+      return true;
+    }
   }
 
   return false;
 }
 
+bool CanShareObjectAcrossIsolates(ObjectPtr obj) {
+  if (!obj->IsHeapObject()) return true;
+  const uword tags = TagsFromUntaggedObject(obj.untag());
+  return CanShareObject(obj, tags);
+}
+
 // Whether executing `get:hashCode` (possibly in a different isolate) on an
 // object with the given [tags] might return a different answer than the source
 // object (if copying is needed) or on the same object (if the object is
diff --git a/runtime/vm/object_graph_copy.h b/runtime/vm/object_graph_copy.h
index e7efa5a..457a23a 100644
--- a/runtime/vm/object_graph_copy.h
+++ b/runtime/vm/object_graph_copy.h
@@ -10,6 +10,10 @@
 class Object;
 class ObjectPtr;
 
+// Whether the object can safely be shared across isolates due to it being
+// deeply immutable.
+bool CanShareObjectAcrossIsolates(ObjectPtr obj);
+
 // Makes a transitive copy of the object graph referenced by [object]. Will not
 // copy objects that can be safely shared - due to being immutable.
 //

To view, visit change 281424. To unsubscribe, or for help writing mail filters, visit settings.

Martin Kustermann (Gerrit)

unread,

Feb 7, 2023, 9:48:44 AM2/7/23

to rev...@dartlang.org, vm-...@dartlang.org, Ryan Macnak, Slava Egorov, Commit Queue

Attention is currently required from: Ryan Macnak, Slava Egorov.

View Change

To view, visit change 281424. To unsubscribe, or for help writing mail filters, visit settings.

Martin Kustermann (Gerrit)

unread,

Feb 7, 2023, 9:49:23 AM2/7/23

to Alexander Aprelev, Slava Egorov, rev...@dartlang.org, vm-...@dartlang.org, Ryan Macnak

Attention is currently required from: Alexander Aprelev, Ryan Macnak.

Martin Kustermann would like Alexander Aprelev to review this change.

Martin Kustermann removed Slava Egorov from this change.

To view, visit change 281424. To unsubscribe, or for help writing mail filters, visit settings.

CBuild (Gerrit)

unread,

Feb 7, 2023, 10:10:15 AM2/7/23

to Martin Kustermann, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, Ryan Macnak, Commit Queue

Attention is currently required from: Alexander Aprelev, Ryan Macnak.

go/dart-cbuild result: SUCCESS

Details: https://goto.google.com/dart-cbuild/find/335001fa2e11fa4430f445d84db466f745fd8f3b

View Change

To view, visit change 281424. To unsubscribe, or for help writing mail filters, visit settings.

Alexander Aprelev (Gerrit)

unread,

Feb 7, 2023, 10:17:32 AM2/7/23

to Martin Kustermann, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, CBuild, Ryan Macnak, Commit Queue

Attention is currently required from: Martin Kustermann, Ryan Macnak.

Patch set 1:Code-Review +1

View Change

3 comments:

Commit Message:
- Patch Set #1, Line 7: deeply
  deeply immutable
- Patch Set #1, Line 18: We do this eagerly to
  Were there any performance benchmarks changes that you noticed?
File runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart:
- Patch Set #1, Line 114: await testUnmodifiableList();
  testSharableTypedData() seems to be missing from this copy of the test, was it intentional?

To view, visit change 281424. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Macnak (Gerrit)

unread,

Feb 7, 2023, 5:54:15 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 1:Code-Review +1

View Change

1 comment:

File runtime/vm/object_graph_copy.cc:
- Patch Set #1, Line 185: }
  I'm somewhat nervous that this bit now means "shallowly immutable if typeddataview, deeply immutable otherwise"

To view, visit change 281424. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward