[vm] Use ClassId in Instance::CanonicalizeHash
We want to reuse CanonicalizeHash for const maps and sets. See design in
go/dart-vm-const-maps.
Before, we only used CanonicalizeHash on a per-class basis. However,
when we start using it for const maps and sets we don't want hash
collisions between const instances with identical layout but a different
class id. So we include the class id into the hash rather than the size.
Bug: https://github.com/dart-lang/sdk/issues/45908
TEST=vm/cc/CanonicalizeHash_Const_Instances
TEST=A ton of bots for existing tests.
Change-Id: Idc0eed49fbae8f76dcfdda21dd9fe2674dc4a240
Cq-Include-Trybots: luci.dart.try:analyzer-nnbd-linux-release-try,app-kernel-linux-debug-x64-try,dart-sdk-linux-try,front-end-nnbd-linux-release-x64-try,pkg-linux-debug-try,vm-canary-linux-debug-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-checked-linux-release-x64-try,vm-kernel-linux-debug-x64c-try,vm-kernel-linux-debug-x64-try,vm-kernel-linux-debug-simarm64c-try,vm-kernel-nnbd-linux-release-simarm-try,vm-kernel-optcounter-threshold-linux-release-x64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208085
Reviewed-by: Tess Strickland <sstrickl@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 1903854..c52f156 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -18961,9 +18961,9 @@
if (Symbol::IsSymbolCid(GetClassId())) {
hash = Symbol::CanonicalizeHash(*this);
} else {
- const intptr_t instance_size = SizeFromClass();
- ASSERT(instance_size != 0);
- hash = instance_size / kCompressedWordSize;
+ const intptr_t class_id = cls.id();
+ ASSERT(class_id != 0);
+ hash = class_id;
uword this_addr = reinterpret_cast<uword>(this->untag());
Object& obj = Object::Handle(zone);
Instance& instance = Instance::Handle(zone);
diff --git a/runtime/vm/object_test.cc b/runtime/vm/object_test.cc
index dae798f..93cfc12 100644
--- a/runtime/vm/object_test.cc
+++ b/runtime/vm/object_test.cc
@@ -5029,6 +5029,64 @@
/*check_identity=*/false));
}
+// Because we want to reuse CanonicalizeHash for hashCode, we should not have
+// collisions.
+TEST_CASE(CanonicalizeHash_Const_Instances) {
+ const char* kScript =
+ "class A {\n"
+ " final int n;\n"
+ " \n"
+ " const A(this.n);\n"
+ "}\n"
+ "\n"
+ "class B {\n"
+ " final int n;\n"
+ " \n"
+ " const B(this.n);\n"
+ "}\n"
+ "\n"
+ "valueA() {\n"
+ " return const A(5);\n"
+ "}\n"
+ "\n"
+ "valueB() {\n"
+ " return const B(5);\n"
+ "}\n";
+
+ Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+ EXPECT_VALID(lib);
+
+ Dart_Handle value_a_result =
+ Dart_Invoke(lib, NewString("valueA"), 0, nullptr);
+ EXPECT_VALID(value_a_result);
+ Dart_Handle value_b_result =
+ Dart_Invoke(lib, NewString("valueB"), 0, nullptr);
+ EXPECT_VALID(value_b_result);
+
+ TransitionNativeToVM transition(Thread::Current());
+
+ const auto& value_a_dart = Instance::CheckedHandle(
+ Thread::Current()->zone(), Api::UnwrapHandle(value_a_result));
+ const auto& value_b_dart = Instance::CheckedHandle(
+ Thread::Current()->zone(), Api::UnwrapHandle(value_b_result));
+
+ const uint32_t canonicalize_hash_a = value_a_dart.CanonicalizeHash();
+ const uint32_t canonicalize_hash_b = value_b_dart.CanonicalizeHash();
+
+ bool success = canonicalize_hash_a != canonicalize_hash_b;
+
+ if (!success) {
+ LogBlock lb;
+ THR_Print("Hash collision between %s and %s\n", value_a_dart.ToCString(),
+ value_b_dart.ToCString());
+ THR_Print("VM CanonicalizeHash a %" Px32 " %" Pd32 "\n",
+ canonicalize_hash_a, canonicalize_hash_a);
+ THR_Print("VM CanonicalizeHash b %" Px32 " %" Pd32 "\n",
+ canonicalize_hash_b, canonicalize_hash_b);
+ }
+ EXPECT(success);
+}
+
TEST_CASE(LinkedHashMap_iteration) {
const char* kScript =
"makeMap() {\n"
