From d0ce6264774dfd3c34b878a0e4588d62f736fd1b Mon Sep 17 00:00:00 2001 From: "mstarzinger@chromium.org" Date: Wed, 26 Oct 2011 12:23:40 +0000 Subject: [PATCH] Fix identity hash code function to respect flag. The flag passed to JSObject::GetIdentityHash() was not respected so far and an indentity hash code was generated even when the flag requested not to do so. This could lead to a rare corner cases (for which a test case was added) where a GC request would have been dropped. R=rossberg@chromium.org TEST=cctest/test-dictionary Review URL: http://codereview.chromium.org/8390047 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@9801 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/objects.cc | 11 +++++-- test/cctest/test-dictionary.cc | 69 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/src/objects.cc b/src/objects.cc index 8562c70..9a87ac5 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -3589,6 +3589,9 @@ MaybeObject* JSObject::GetIdentityHash(CreationFlag flag) { Object* stored_value = GetHiddenProperty(GetHeap()->identity_hash_symbol()); if (stored_value->IsSmi()) return stored_value; + // Do not generate permanent identity hash code if not requested. + if (flag == OMIT_CREATION) return GetHeap()->undefined_value(); + Smi* hash = GenerateIdentityHash(); MaybeObject* result = SetHiddenProperty(GetHeap()->identity_hash_symbol(), hash); @@ -12390,7 +12393,7 @@ MaybeObject* StringDictionary::TransformPropertiesToFastFor( bool ObjectHashSet::Contains(Object* key) { // If the object does not have an identity hash, it was never used as a key. { MaybeObject* maybe_hash = key->GetHash(OMIT_CREATION); - if (maybe_hash->IsFailure()) return false; + if (maybe_hash->ToObjectUnchecked()->IsUndefined()) return false; } return (FindEntry(key) != kNotFound); } @@ -12424,7 +12427,7 @@ MaybeObject* ObjectHashSet::Add(Object* key) { MaybeObject* ObjectHashSet::Remove(Object* key) { // If the object does not have an identity hash, it was never used as a key. { MaybeObject* maybe_hash = key->GetHash(OMIT_CREATION); - if (maybe_hash->IsFailure()) return this; + if (maybe_hash->ToObjectUnchecked()->IsUndefined()) return this; } int entry = FindEntry(key); @@ -12441,7 +12444,9 @@ MaybeObject* ObjectHashSet::Remove(Object* key) { Object* ObjectHashTable::Lookup(Object* key) { // If the object does not have an identity hash, it was never used as a key. { MaybeObject* maybe_hash = key->GetHash(OMIT_CREATION); - if (maybe_hash->IsFailure()) GetHeap()->undefined_value(); + if (maybe_hash->ToObjectUnchecked()->IsUndefined()) { + return GetHeap()->undefined_value(); + } } int entry = FindEntry(key); if (entry == kNotFound) return GetHeap()->undefined_value(); diff --git a/test/cctest/test-dictionary.cc b/test/cctest/test-dictionary.cc index 15a854b..793e228 100644 --- a/test/cctest/test-dictionary.cc +++ b/test/cctest/test-dictionary.cc @@ -38,6 +38,7 @@ using namespace v8::internal; + TEST(ObjectHashTable) { v8::HandleScope scope; LocalContext context; @@ -66,7 +67,8 @@ TEST(ObjectHashTable) { CHECK_EQ(table->NumberOfDeletedElements(), 1); CHECK_EQ(table->Lookup(*a), HEAP->undefined_value()); - // Keys should map back to their respective values. + // Keys should map back to their respective values and also should get + // an identity hash code generated. for (int i = 0; i < 100; i++) { Handle key = FACTORY->NewJSArray(7); Handle value = FACTORY->NewJSArray(11); @@ -74,12 +76,67 @@ TEST(ObjectHashTable) { CHECK_EQ(table->NumberOfElements(), i + 1); CHECK_NE(table->FindEntry(*key), ObjectHashTable::kNotFound); CHECK_EQ(table->Lookup(*key), *value); + CHECK(key->GetIdentityHash(OMIT_CREATION)->ToObjectChecked()->IsSmi()); + } + + // Keys never added to the map which already have an identity hash + // code should not be found. + for (int i = 0; i < 100; i++) { + Handle key = FACTORY->NewJSArray(7); + CHECK(key->GetIdentityHash(ALLOW_CREATION)->ToObjectChecked()->IsSmi()); + CHECK_EQ(table->FindEntry(*key), ObjectHashTable::kNotFound); + CHECK_EQ(table->Lookup(*key), HEAP->undefined_value()); + CHECK(key->GetIdentityHash(OMIT_CREATION)->ToObjectChecked()->IsSmi()); } - // Keys never added to the map should not be found. - for (int i = 0; i < 1000; i++) { - Handle o = FACTORY->NewJSArray(100); - CHECK_EQ(table->FindEntry(*o), ObjectHashTable::kNotFound); - CHECK_EQ(table->Lookup(*o), HEAP->undefined_value()); + // Keys that don't have an identity hash should not be found and also + // should not get an identity hash code generated. + for (int i = 0; i < 100; i++) { + Handle key = FACTORY->NewJSArray(7); + CHECK_EQ(table->Lookup(*key), HEAP->undefined_value()); + CHECK_EQ(key->GetIdentityHash(OMIT_CREATION), HEAP->undefined_value()); } } + + +#ifdef DEBUG +TEST(ObjectHashSetCausesGC) { + v8::HandleScope scope; + LocalContext context; + Handle table = FACTORY->NewObjectHashSet(1); + Handle key = FACTORY->NewJSArray(0); + + // Simulate a full heap so that generating an identity hash code + // in subsequent calls will request GC. + FLAG_gc_interval = 0; + + // Calling Contains() should not cause GC ever. + CHECK(!table->Contains(*key)); + + // Calling Remove() should not cause GC ever. + CHECK(!table->Remove(*key)->IsFailure()); + + // Calling Add() should request GC by returning a failure. + CHECK(table->Add(*key)->IsRetryAfterGC()); +} +#endif + + +#ifdef DEBUG +TEST(ObjectHashTableCausesGC) { + v8::HandleScope scope; + LocalContext context; + Handle table = FACTORY->NewObjectHashTable(1); + Handle key = FACTORY->NewJSArray(0); + + // Simulate a full heap so that generating an identity hash code + // in subsequent calls will request GC. + FLAG_gc_interval = 0; + + // Calling Lookup() should not cause GC ever. + CHECK(table->Lookup(*key)->IsUndefined()); + + // Calling Put() should request GC by returning a failure. + CHECK(table->Put(*key, *key)->IsRetryAfterGC()); +} +#endif -- 2.7.4