From aeec653f49b09fb4cda3f7b1090a7baf214c7795 Mon Sep 17 00:00:00 2001 From: yurys Date: Tue, 3 Feb 2015 06:42:34 -0800 Subject: [PATCH] Revert of Add WeakMap to v8.h (patchset #3 id:40001 of https://codereview.chromium.org/886473005/) Reason for revert: Broke compilation on component build http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20nosnap%20-%20shared/builds/5007/steps/compile/logs/stdio Original issue's description: > Add WeakMap to v8.h > > A new map wich references its keys weakly is added to v8.h. Internally it uses the same storage as JSWeakMap but doesn't depend on the JavaScript part of WeakMap implementation in weak-collection.js, hence it can be instantiated without entering any context. > > BUG=chromium:437416 > LOG=Y > > Committed: https://crrev.com/37d4c57630636f21e3add8d3d1c7c978ff5fc8e0 > Cr-Commit-Position: refs/heads/master@{#26401} TBR=jochen@chromium.org,mstarzinger@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:437416 Review URL: https://codereview.chromium.org/898763002 Cr-Commit-Position: refs/heads/master@{#26402} --- include/v8.h | 25 --------- src/api.cc | 106 ------------------------------------- src/factory.cc | 9 ---- src/factory.h | 2 - src/runtime/runtime-collections.cc | 58 ++++++++------------ src/runtime/runtime.h | 7 --- test/cctest/test-api.cc | 73 ------------------------- test/cctest/test-weakmaps.cc | 11 +++- test/cctest/test-weaksets.cc | 6 ++- 9 files changed, 35 insertions(+), 262 deletions(-) diff --git a/include/v8.h b/include/v8.h index d1c476a..e8ca3df 100644 --- a/include/v8.h +++ b/include/v8.h @@ -1536,31 +1536,6 @@ class V8_EXPORT JSON { }; -/** - * A map whose keys are referenced weakly. It is similar to JavaScript WeakMap - * but can be created without entering a v8::Context and hence shouldn't - * escape to JavaScript. - */ -class V8_EXPORT WeakMap { - public: - static WeakMap* New(Isolate* isolate); - void Set(Handle key, Handle value); - Local Get(Handle key); - bool Has(Handle key); - bool Delete(Handle key); - - private: - WeakMap(Isolate* isolate, Handle weak_map) - : isolate_(isolate), map_(isolate, weak_map) {} - Isolate* isolate_; - UniquePersistent map_; - - // Disallow copying and assigning. - WeakMap(WeakMap&); - void operator=(WeakMap&); -}; - - // --- Value --- diff --git a/src/api.cc b/src/api.cc index 0030e09..6caa82a 100644 --- a/src/api.cc +++ b/src/api.cc @@ -2249,112 +2249,6 @@ bool StackFrame::IsConstructor() const { } -// --- W e a k M a p --- - -WeakMap* WeakMap::New(Isolate* v8_isolate) { - i::Isolate* isolate = reinterpret_cast(v8_isolate); - ENTER_V8(isolate); - i::HandleScope scope(isolate); - i::Handle weakmap = isolate->factory()->NewJSWeakMap(); - i::Runtime::WeakCollectionInitialize(isolate, weakmap); - Local v8_obj = Utils::ToLocal(i::Handle::cast(weakmap)); - return new WeakMap(v8_isolate, v8_obj); -} - - -void WeakMap::Set(Handle v8_key, Handle v8_value) { - v8::HandleScope handleScope(isolate_); - Local map_handle = Local::New(isolate_, map_); - i::Isolate* isolate = reinterpret_cast(isolate_); - ENTER_V8(isolate); - i::Handle key = Utils::OpenHandle(*v8_key); - i::Handle value = Utils::OpenHandle(*v8_value); - i::Handle weak_collection = - i::Handle::cast(Utils::OpenHandle(*map_handle)); - if (!key->IsJSReceiver() && !key->IsSymbol()) { - DCHECK(false); - return; - } - i::Handle table( - i::ObjectHashTable::cast(weak_collection->table())); - if (!table->IsKey(*key)) { - DCHECK(false); - return; - } - i::Runtime::WeakCollectionSet(weak_collection, key, value); -} - - -Local WeakMap::Get(Handle v8_key) { - v8::EscapableHandleScope handleScope(isolate_); - Local map_handle = Local::New(isolate_, map_); - i::Isolate* isolate = reinterpret_cast(isolate_); - ENTER_V8(isolate); - i::Handle key = Utils::OpenHandle(*v8_key); - i::Handle weak_collection = - i::Handle::cast(Utils::OpenHandle(*map_handle)); - if (!key->IsJSReceiver() && !key->IsSymbol()) { - DCHECK(false); - return Undefined(isolate_); - } - i::Handle table( - i::ObjectHashTable::cast(weak_collection->table())); - if (!table->IsKey(*key)) { - DCHECK(false); - return Undefined(isolate_); - } - i::Handle lookup(table->Lookup(key), isolate); - if (lookup->IsTheHole()) return Undefined(isolate_); - Local result = Utils::ToLocal(lookup); - return handleScope.Escape(result); -} - - -bool WeakMap::Has(Handle v8_key) { - v8::HandleScope handleScope(isolate_); - Local map_handle = Local::New(isolate_, map_); - i::Isolate* isolate = reinterpret_cast(isolate_); - ENTER_V8(isolate); - i::Handle key = Utils::OpenHandle(*v8_key); - i::Handle weak_collection = - i::Handle::cast(Utils::OpenHandle(*map_handle)); - if (!key->IsJSReceiver() && !key->IsSymbol()) { - DCHECK(false); - return false; - } - i::Handle table( - i::ObjectHashTable::cast(weak_collection->table())); - if (!table->IsKey(*key)) { - DCHECK(false); - return false; - } - i::Handle lookup(table->Lookup(key), isolate); - return !lookup->IsTheHole(); -} - - -bool WeakMap::Delete(Handle v8_key) { - v8::HandleScope handleScope(isolate_); - Local map_handle = Local::New(isolate_, map_); - i::Isolate* isolate = reinterpret_cast(isolate_); - ENTER_V8(isolate); - i::Handle key = Utils::OpenHandle(*v8_key); - i::Handle weak_collection = - i::Handle::cast(Utils::OpenHandle(*map_handle)); - if (!key->IsJSReceiver() && !key->IsSymbol()) { - DCHECK(false); - return false; - } - i::Handle table( - i::ObjectHashTable::cast(weak_collection->table())); - if (!table->IsKey(*key)) { - DCHECK(false); - return false; - } - return i::Runtime::WeakCollectionDelete(weak_collection, key); -} - - // --- J S O N --- Local JSON::Parse(Local json_string) { diff --git a/src/factory.cc b/src/factory.cc index c1c818c..8226a0e 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -2236,15 +2236,6 @@ Handle Factory::NewArgumentsObject(Handle callee, } -Handle Factory::NewJSWeakMap() { - // TODO(adamk): Currently the map is only created three times per - // isolate. If it's created more often, the map should be moved into the - // strong root list. - Handle map = NewMap(JS_WEAK_MAP_TYPE, JSWeakMap::kSize); - return Handle::cast(NewJSObjectFromMap(map)); -} - - Handle Factory::CreateApiFunction( Handle obj, Handle prototype, diff --git a/src/factory.h b/src/factory.h index 5be5c12..141fc5e 100644 --- a/src/factory.h +++ b/src/factory.h @@ -359,8 +359,6 @@ class Factory FINAL { return NewJSObjectFromMap(neander_map()); } - Handle NewJSWeakMap(); - Handle NewArgumentsObject(Handle callee, int length); // JS objects are pretenured when allocated by the bootstrapper and diff --git a/src/runtime/runtime-collections.cc b/src/runtime/runtime-collections.cc index ffffbdd..abdd056 100644 --- a/src/runtime/runtime-collections.cc +++ b/src/runtime/runtime-collections.cc @@ -296,11 +296,12 @@ RUNTIME_FUNCTION(Runtime_MapIteratorNext) { } -void Runtime::WeakCollectionInitialize( +static Handle WeakCollectionInitialize( Isolate* isolate, Handle weak_collection) { DCHECK(weak_collection->map()->inobject_properties() == 0); Handle table = ObjectHashTable::New(isolate, 0); weak_collection->set_table(*table); + return weak_collection; } @@ -308,8 +309,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionInitialize) { HandleScope scope(isolate); DCHECK(args.length() == 1); CONVERT_ARG_HANDLE_CHECKED(JSWeakCollection, weak_collection, 0); - Runtime::WeakCollectionInitialize(isolate, weak_collection); - return *weak_collection; + return *WeakCollectionInitialize(isolate, weak_collection); } @@ -341,24 +341,6 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionHas) { } -bool Runtime::WeakCollectionDelete(Handle weak_collection, - Handle key) { - DCHECK(key->IsJSReceiver() || key->IsSymbol()); - Handle table( - ObjectHashTable::cast(weak_collection->table())); - DCHECK(table->IsKey(*key)); - bool was_present = false; - Handle new_table = - ObjectHashTable::Remove(table, key, &was_present); - weak_collection->set_table(*new_table); - if (*table != *new_table) { - // Zap the old table since we didn't record slots for its elements. - table->FillWithHoles(0, table->length()); - } - return was_present; -} - - RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) { HandleScope scope(isolate); DCHECK(args.length() == 2); @@ -368,23 +350,15 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) { Handle table( ObjectHashTable::cast(weak_collection->table())); RUNTIME_ASSERT(table->IsKey(*key)); - bool was_present = Runtime::WeakCollectionDelete(weak_collection, key); - return isolate->heap()->ToBoolean(was_present); -} - - -void Runtime::WeakCollectionSet(Handle weak_collection, - Handle key, Handle value) { - DCHECK(key->IsJSReceiver() || key->IsSymbol()); - Handle table( - ObjectHashTable::cast(weak_collection->table())); - DCHECK(table->IsKey(*key)); - Handle new_table = ObjectHashTable::Put(table, key, value); + bool was_present = false; + Handle new_table = + ObjectHashTable::Remove(table, key, &was_present); weak_collection->set_table(*new_table); if (*table != *new_table) { // Zap the old table since we didn't record slots for its elements. table->FillWithHoles(0, table->length()); } + return isolate->heap()->ToBoolean(was_present); } @@ -398,7 +372,12 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionSet) { Handle table( ObjectHashTable::cast(weak_collection->table())); RUNTIME_ASSERT(table->IsKey(*key)); - Runtime::WeakCollectionSet(weak_collection, key, value); + Handle new_table = ObjectHashTable::Put(table, key, value); + weak_collection->set_table(*new_table); + if (*table != *new_table) { + // Zap the old table since we didn't record slots for its elements. + table->FillWithHoles(0, table->length()); + } return *weak_collection; } @@ -435,9 +414,14 @@ RUNTIME_FUNCTION(Runtime_GetWeakSetValues) { RUNTIME_FUNCTION(Runtime_ObservationWeakMapCreate) { HandleScope scope(isolate); DCHECK(args.length() == 0); - Handle weakmap = isolate->factory()->NewJSWeakMap(); - Runtime::WeakCollectionInitialize(isolate, weakmap); - return *weakmap; + // TODO(adamk): Currently this runtime function is only called three times per + // isolate. If it's called more often, the map should be moved into the + // strong root list. + Handle map = + isolate->factory()->NewMap(JS_WEAK_MAP_TYPE, JSWeakMap::kSize); + Handle weakmap = + Handle::cast(isolate->factory()->NewJSObjectFromMap(map)); + return *WeakCollectionInitialize(isolate, weakmap); } } } // namespace v8::internal diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index 3385215..dca28f1 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -879,13 +879,6 @@ class Runtime : public AllStatic { MUST_USE_RESULT static MaybeHandle CreateArrayLiteralBoilerplate( Isolate* isolate, Handle literals, Handle elements); - - static void WeakCollectionInitialize( - Isolate* isolate, Handle weak_collection); - static void WeakCollectionSet(Handle weak_collection, - Handle key, Handle value); - static bool WeakCollectionDelete(Handle weak_collection, - Handle key); }; diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 0d2c71e..0009253 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -46,7 +46,6 @@ #include "src/isolate.h" #include "src/objects.h" #include "src/parser.h" -#include "src/smart-pointers.h" #include "src/snapshot.h" #include "src/unicode-inl.h" #include "src/utils.h" @@ -4738,78 +4737,6 @@ TEST(MessageHandler5) { } -TEST(WeakMap) { - v8::Isolate* isolate = CcTest::isolate(); - i::SmartPointer weak_map(v8::WeakMap::New(isolate)); - CHECK(!weak_map.is_empty()); - - LocalContext env; - HandleScope scope(isolate); - - Local value = v8::Object::New(isolate); - - Local local1 = v8::Object::New(isolate); - CHECK(!weak_map->Has(local1)); - CHECK(weak_map->Get(local1)->IsUndefined()); - weak_map->Set(local1, value); - CHECK(weak_map->Has(local1)); - CHECK(value->Equals(weak_map->Get(local1))); - - WeakCallCounter counter(1234); - WeakCallCounterAndPersistent o1(&counter); - WeakCallCounterAndPersistent o2(&counter); - WeakCallCounterAndPersistent s1(&counter); - { - HandleScope scope(isolate); - Local obj1 = v8::Object::New(isolate); - Local obj2 = v8::Object::New(isolate); - Local sym1 = v8::Symbol::New(isolate); - - weak_map->Set(obj1, value); - weak_map->Set(obj2, value); - weak_map->Set(sym1, value); - - o1.handle.Reset(isolate, obj1); - o2.handle.Reset(isolate, obj2); - s1.handle.Reset(isolate, sym1); - - CHECK(weak_map->Has(local1)); - CHECK(weak_map->Has(obj1)); - CHECK(weak_map->Has(obj2)); - CHECK(weak_map->Has(sym1)); - - CHECK(value->Equals(weak_map->Get(local1))); - CHECK(value->Equals(weak_map->Get(obj1))); - CHECK(value->Equals(weak_map->Get(obj2))); - CHECK(value->Equals(weak_map->Get(sym1))); - } - CcTest::heap()->CollectAllGarbage(TestHeap::Heap::kNoGCFlags); - { - HandleScope scope(isolate); - CHECK(value->Equals(weak_map->Get(local1))); - CHECK(value->Equals(weak_map->Get(Local::New(isolate, o1.handle)))); - CHECK(value->Equals(weak_map->Get(Local::New(isolate, o2.handle)))); - CHECK(value->Equals(weak_map->Get(Local::New(isolate, s1.handle)))); - } - - o1.handle.SetWeak(&o1, &WeakPointerCallback); - o2.handle.SetWeak(&o2, &WeakPointerCallback); - s1.handle.SetWeak(&s1, &WeakPointerCallback); - - CcTest::heap()->CollectAllGarbage(TestHeap::Heap::kNoGCFlags); - CHECK_EQ(3, counter.NumberOfWeakCalls()); - - CHECK(o1.handle.IsEmpty()); - CHECK(o2.handle.IsEmpty()); - CHECK(s1.handle.IsEmpty()); - - CHECK(value->Equals(weak_map->Get(local1))); - CHECK(weak_map->Delete(local1)); - CHECK(!weak_map->Has(local1)); - CHECK(weak_map->Get(local1)->IsUndefined()); -} - - THREADED_TEST(GetSetProperty) { LocalContext context; v8::Isolate* isolate = context->GetIsolate(); diff --git a/test/cctest/test-weakmaps.cc b/test/cctest/test-weakmaps.cc index c3b0c2e..6c19cb8 100644 --- a/test/cctest/test-weakmaps.cc +++ b/test/cctest/test-weakmaps.cc @@ -42,7 +42,10 @@ static Isolate* GetIsolateFrom(LocalContext* context) { static Handle AllocateJSWeakMap(Isolate* isolate) { - Handle weakmap = isolate->factory()->NewJSWeakMap(); + Factory* factory = isolate->factory(); + Handle map = factory->NewMap(JS_WEAK_MAP_TYPE, JSWeakMap::kSize); + Handle weakmap_obj = factory->NewJSObjectFromMap(map); + Handle weakmap(JSWeakMap::cast(*weakmap_obj)); // Do not leak handles for the hash table, it would make entries strong. { HandleScope scope(isolate); @@ -55,7 +58,11 @@ static Handle AllocateJSWeakMap(Isolate* isolate) { static void PutIntoWeakMap(Handle weakmap, Handle key, Handle value) { - Runtime::WeakCollectionSet(weakmap, key, value); + Handle table = ObjectHashTable::Put( + Handle(ObjectHashTable::cast(weakmap->table())), + Handle(JSObject::cast(*key)), + value); + weakmap->set_table(*table); } static int NumberOfWeakCalls = 0; diff --git a/test/cctest/test-weaksets.cc b/test/cctest/test-weaksets.cc index 60f2d45..299cc92 100644 --- a/test/cctest/test-weaksets.cc +++ b/test/cctest/test-weaksets.cc @@ -58,7 +58,11 @@ static Handle AllocateJSWeakSet(Isolate* isolate) { static void PutIntoWeakSet(Handle weakset, Handle key, Handle value) { - Runtime::WeakCollectionSet(weakset, key, value); + Handle table = ObjectHashTable::Put( + Handle(ObjectHashTable::cast(weakset->table())), + Handle(JSObject::cast(*key)), + value); + weakset->set_table(*table); } static int NumberOfWeakCalls = 0; -- 2.7.4