Skip to content

Commit 275d956

Browse files
joyeecheungtargos
authored andcommitted
deps: V8: cherry-pick 0ce9f16abc1f
Original commit message: [runtime] sort transition array after it grows beyond linear search size Previously, after the TransitionArray is rehashed, TransitionsAccessor::InsertHelper() can grow the transition array without making sure it's sorted after the size crosses kMaxElementsForLinearSearch, leading to search failures or duplicate entries in bigger transition arrays. See #61002 (comment) Change-Id: I3f45816b7d65bbbcfedeb7074d0c6907cbc08edf Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7252791 Commit-Queue: Joyee Cheung <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/main@{#104384} Refs: v8/v8@0ce9f16
1 parent b5dbf97 commit 275d956

File tree

3 files changed

+159
-1
lines changed

3 files changed

+159
-1
lines changed

common.gypi

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
# Reset this number to 0 on major V8 upgrades.
4040
# Increment by one for each non-official patch applied to deps/v8.
41-
'v8_embedder_string': '-node.8',
41+
'v8_embedder_string': '-node.9',
4242

4343
##### V8 defaults for Node.js #####
4444

deps/v8/src/objects/transitions.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,11 @@ void TransitionsAccessor::InsertHelper(Isolate* isolate, DirectHandle<Map> map,
173173
}
174174
array->SetKey(insertion_index, *name);
175175
array->SetRawTarget(insertion_index, MakeWeak(*target));
176+
// The new size exceeds the threshold for linear search, sort the array
177+
// for binary search later.
178+
if (new_nof == TransitionArray::kMaxElementsForLinearSearch + 1) {
179+
array->Sort();
180+
}
176181
SLOW_DCHECK(array->IsSortedNoDuplicates());
177182
return;
178183
}
@@ -222,6 +227,11 @@ void TransitionsAccessor::InsertHelper(Isolate* isolate, DirectHandle<Map> map,
222227
result->Set(i + 1, array->GetKey(i), array->GetRawTarget(i));
223228
}
224229

230+
// The new size exceeds the threshold for linear search, sort the array
231+
// for binary search later.
232+
if (new_nof == TransitionArray::kMaxElementsForLinearSearch + 1) {
233+
result->Sort();
234+
}
225235
SLOW_DCHECK(result->IsSortedNoDuplicates());
226236
ReplaceTransitions(isolate, map, result);
227237
}

deps/v8/test/cctest/test-transitions.cc

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@
88

99
#include <utility>
1010

11+
#include "src/api/api-inl.h"
1112
#include "src/codegen/compilation-cache.h"
1213
#include "src/execution/execution.h"
1314
#include "src/heap/factory.h"
15+
#include "src/numbers/hash-seed-inl.h"
1416
#include "src/objects/field-type.h"
1517
#include "src/objects/objects-inl.h"
1618
#include "src/objects/transitions-inl.h"
@@ -316,5 +318,151 @@ TEST(TransitionArray_SameFieldNamesDifferentAttributes) {
316318
DCHECK(transitions.IsSortedNoDuplicates());
317319
}
318320

321+
UNINITIALIZED_TEST(TransitionArray_InsertToBinarySearchSizeAfterRehashing) {
322+
v8_flags.rehash_snapshot = true;
323+
v8_flags.hash_seed = 42;
324+
v8_flags.allow_natives_syntax = true;
325+
DisableEmbeddedBlobRefcounting();
326+
v8::StartupData blob;
327+
int initial_size = TransitionArray::kMaxElementsForLinearSearch - 3;
328+
int final_size = TransitionArray::kMaxElementsForLinearSearch + 3;
329+
330+
{
331+
v8::Isolate::CreateParams testing_params;
332+
testing_params.array_buffer_allocator = CcTest::array_buffer_allocator();
333+
v8::SnapshotCreator creator(testing_params);
334+
v8::Isolate* isolate = creator.GetIsolate();
335+
{
336+
v8::HandleScope handle_scope(isolate);
337+
v8::Local<v8::Context> context = v8::Context::New(isolate);
338+
v8::Context::Scope context_scope(context);
339+
// Get the cached map for empty objects
340+
Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate);
341+
v8::Local<v8::Object> obj = v8::Object::New(isolate);
342+
DirectHandle<Map> first_map =
343+
direct_handle(v8::Utils::OpenDirectHandle(*obj)->map(), i_isolate);
344+
345+
// Ensure that the transition array is empty initially.
346+
{
347+
TestTransitionsAccessor transitions(i_isolate, first_map);
348+
CHECK_EQ(0, transitions.NumberOfTransitions());
349+
DCHECK(transitions.transitions()->IsSortedNoDuplicates());
350+
}
351+
352+
// Insert some transitions to grow the transition array, we'll rehash
353+
// later using the snapshot which "shuffles" the entries in hash order.
354+
v8::Local<v8::Value> null_value = v8::Null(isolate);
355+
v8::LocalVector<v8::Value> objects(isolate);
356+
for (int i = 0; i < initial_size; i++) {
357+
std::string prop_name = "prop_" + std::to_string(i);
358+
v8::Local<v8::String> name =
359+
v8::String::NewFromUtf8(isolate, prop_name.c_str(),
360+
v8::NewStringType::kNormal)
361+
.ToLocalChecked();
362+
v8::Local<v8::Object> new_obj = v8::Object::New(isolate);
363+
new_obj->Set(context, name, null_value).Check();
364+
objects.push_back(new_obj);
365+
}
366+
context->Global()
367+
->Set(context, v8_str("objects_for_transitions"),
368+
v8::Array::New(isolate, objects.data(), objects.size()))
369+
.Check();
370+
371+
creator.SetDefaultContext(context);
372+
373+
// Verify that the cached map has initial_size transitions.
374+
TestTransitionsAccessor transitions(i_isolate, first_map);
375+
CHECK_EQ(initial_size, transitions.NumberOfTransitions());
376+
DCHECK(transitions.transitions()->IsSortedNoDuplicates());
377+
}
378+
blob =
379+
creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear);
380+
CHECK(blob.CanBeRehashed());
381+
}
382+
383+
// Create a new isolate from the snapshot with rehashing enabled.
384+
v8_flags.hash_seed = 1337;
385+
v8::Isolate::CreateParams testing_params;
386+
testing_params.array_buffer_allocator = CcTest::array_buffer_allocator();
387+
testing_params.snapshot_blob = &blob;
388+
v8::Isolate* isolate = v8::Isolate::New(testing_params);
389+
{
390+
v8::Isolate::Scope isolate_scope(isolate);
391+
// Check that rehashing has been performed.
392+
CHECK_EQ(static_cast<uint64_t>(1337),
393+
HashSeed(reinterpret_cast<Isolate*>(isolate)).seed());
394+
v8::HandleScope handle_scope(isolate);
395+
v8::Local<v8::Context> context = v8::Context::New(isolate);
396+
CHECK(!context.IsEmpty());
397+
v8::Context::Scope context_scope(context);
398+
399+
// Get the cached map for empty objects.
400+
Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate);
401+
v8::Local<v8::Object> obj = v8::Object::New(isolate);
402+
DirectHandle<Map> first_map =
403+
direct_handle(v8::Utils::OpenDirectHandle(*obj)->map(), i_isolate);
404+
405+
// Verify that the cached map still has initial_size transitions.
406+
{
407+
TestTransitionsAccessor transitions(i_isolate, first_map);
408+
CHECK_EQ(initial_size, transitions.NumberOfTransitions());
409+
DCHECK(transitions.transitions()->IsSortedNoDuplicates());
410+
}
411+
412+
v8::Local<v8::Value> null_value = v8::Null(isolate);
413+
{
414+
v8::LocalVector<v8::Value> objects(isolate);
415+
416+
// Insert some transitions to grow the rehashed transition array beyond
417+
// linear search size
418+
for (int i = initial_size; i < final_size; i++) {
419+
std::string prop_name = "prop_" + std::to_string(i);
420+
v8::Local<v8::String> name =
421+
v8::String::NewFromUtf8(isolate, prop_name.c_str(),
422+
v8::NewStringType::kNormal)
423+
.ToLocalChecked();
424+
v8::Local<v8::Object> new_obj = v8::Object::New(isolate);
425+
new_obj->Set(context, name, null_value).Check();
426+
objects.push_back(new_obj);
427+
}
428+
context->Global()
429+
->Set(context, v8_str("objects_for_transitions_2"),
430+
v8::Array::New(isolate, objects.data(), objects.size()))
431+
.Check();
432+
TestTransitionsAccessor transitions(i_isolate, first_map);
433+
CHECK_EQ(final_size, transitions.NumberOfTransitions());
434+
DCHECK(transitions.transitions()->IsSortedNoDuplicates());
435+
}
436+
437+
// Check existing transitions could be found after rehashing.
438+
{
439+
v8::LocalVector<v8::Value> objects(isolate);
440+
for (int i = 0; i < final_size; i++) {
441+
std::string prop_name = "prop_" + std::to_string(i);
442+
v8::Local<v8::String> name =
443+
v8::String::NewFromUtf8(isolate, prop_name.c_str(),
444+
v8::NewStringType::kNormal)
445+
.ToLocalChecked();
446+
v8::Local<v8::Object> new_obj = v8::Object::New(isolate);
447+
new_obj->Set(context, name, null_value).Check();
448+
objects.push_back(new_obj);
449+
}
450+
451+
context->Global()
452+
->Set(context, v8_str("objects_for_transitions_3"),
453+
v8::Array::New(isolate, objects.data(), objects.size()))
454+
.Check();
455+
456+
TestTransitionsAccessor transitions(i_isolate, first_map);
457+
CHECK_EQ(final_size, transitions.NumberOfTransitions());
458+
DCHECK(transitions.transitions()->IsSortedNoDuplicates());
459+
}
460+
}
461+
462+
isolate->Dispose();
463+
delete[] blob.data;
464+
FreeCurrentEmbeddedBlob();
465+
}
466+
319467
} // namespace internal
320468
} // namespace v8

0 commit comments

Comments
 (0)