From 4b6981f74d42c27383017184b19d052990ed1327 Mon Sep 17 00:00:00 2001 From: "antonm@chromium.org" Date: Mon, 17 Jan 2011 16:54:56 +0000 Subject: [PATCH] Do not use possibly stale values for cache size, etc. Those value can become invalid if cache gets cleared by GC. Review URL: http://codereview.chromium.org/6348002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6353 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/objects-debug.cc | 13 +++--- src/objects-inl.h | 26 ++++++++++-- src/objects.h | 5 +++ src/runtime.cc | 112 ++++++++++++++++++++++++++------------------------- 4 files changed, 93 insertions(+), 63 deletions(-) diff --git a/src/objects-debug.cc b/src/objects-debug.cc index 7d50bfb..f9c57e6 100644 --- a/src/objects-debug.cc +++ b/src/objects-debug.cc @@ -670,16 +670,17 @@ void JSFunctionResultCache::JSFunctionResultCacheVerify() { int finger = Smi::cast(get(kFingerIndex))->value(); ASSERT(kEntriesIndex <= finger); - ASSERT(finger < size || finger == kEntriesIndex); + ASSERT((finger < size) || (finger == kEntriesIndex && finger == size)); ASSERT_EQ(0, finger % kEntrySize); if (FLAG_enable_slow_asserts) { - STATIC_ASSERT(2 == kEntrySize); - for (int i = kEntriesIndex; i < length(); i += kEntrySize) { + for (int i = kEntriesIndex; i < size; i++) { + ASSERT(!get(i)->IsTheHole()); + get(i)->Verify(); + } + for (int i = size; i < length(); i++) { + ASSERT(get(i)->IsTheHole()); get(i)->Verify(); - get(i + 1)->Verify(); - // Key and value must be either both the holes, or not. - ASSERT(get(i)->IsTheHole() == get(i + 1)->IsTheHole()); } } } diff --git a/src/objects-inl.h b/src/objects-inl.h index abfd443..de0ffa3 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -1978,13 +1978,13 @@ void ExternalTwoByteString::set_resource( void JSFunctionResultCache::MakeZeroSize() { - set(kFingerIndex, Smi::FromInt(kEntriesIndex)); - set(kCacheSizeIndex, Smi::FromInt(kEntriesIndex)); + set_finger_index(kEntriesIndex); + set_size(kEntriesIndex); } void JSFunctionResultCache::Clear() { - int cache_size = Smi::cast(get(kCacheSizeIndex))->value(); + int cache_size = size(); Object** entries_start = RawField(this, OffsetOfElementAt(kEntriesIndex)); MemsetPointer(entries_start, Heap::the_hole_value(), @@ -1993,6 +1993,26 @@ void JSFunctionResultCache::Clear() { } +int JSFunctionResultCache::size() { + return Smi::cast(get(kCacheSizeIndex))->value(); +} + + +void JSFunctionResultCache::set_size(int size) { + set(kCacheSizeIndex, Smi::FromInt(size)); +} + + +int JSFunctionResultCache::finger_index() { + return Smi::cast(get(kFingerIndex))->value(); +} + + +void JSFunctionResultCache::set_finger_index(int finger_index) { + set(kFingerIndex, Smi::FromInt(finger_index)); +} + + byte ByteArray::get(int index) { ASSERT(index >= 0 && index < this->length()); return READ_BYTE_FIELD(this, kHeaderSize + index * kCharSize); diff --git a/src/objects.h b/src/objects.h index c136dc5..9a577f1 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2613,6 +2613,11 @@ class JSFunctionResultCache: public FixedArray { inline void MakeZeroSize(); inline void Clear(); + inline int size(); + inline void set_size(int size); + inline int finger_index(); + inline void set_finger_index(int finger_index); + // Casting static inline JSFunctionResultCache* cast(Object* obj); diff --git a/src/runtime.cc b/src/runtime.cc index 0cde777..2f1f54c 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -10654,51 +10654,12 @@ static MaybeObject* Runtime_Abort(Arguments args) { } -MUST_USE_RESULT static MaybeObject* CacheMiss(FixedArray* cache_obj, - int index, - Object* key_obj) { - ASSERT(index % 2 == 0); // index of the key - ASSERT(index >= JSFunctionResultCache::kEntriesIndex); - ASSERT(index < cache_obj->length()); - - HandleScope scope; - - Handle cache(cache_obj); - Handle key(key_obj); - Handle factory(JSFunction::cast( - cache->get(JSFunctionResultCache::kFactoryIndex))); - // TODO(antonm): consider passing a receiver when constructing a cache. - Handle receiver(Top::global_context()->global()); - - Handle value; - { - // This handle is nor shared, nor used later, so it's safe. - Object** argv[] = { key.location() }; - bool pending_exception = false; - value = Execution::Call(factory, - receiver, - 1, - argv, - &pending_exception); - if (pending_exception) return Failure::Exception(); - } - - cache->set(index, *key); - cache->set(index + 1, *value); - cache->set(JSFunctionResultCache::kFingerIndex, Smi::FromInt(index)); - - return *value; -} - - static MaybeObject* Runtime_GetFromCache(Arguments args) { // This is only called from codegen, so checks might be more lax. - CONVERT_CHECKED(FixedArray, cache, args[0]); + CONVERT_CHECKED(JSFunctionResultCache, cache, args[0]); Object* key = args[1]; - const int finger_index = - Smi::cast(cache->get(JSFunctionResultCache::kFingerIndex))->value(); - + int finger_index = cache->finger_index(); Object* o = cache->get(finger_index); if (o == key) { // The fastest case: hit the same place again. @@ -10710,35 +10671,78 @@ static MaybeObject* Runtime_GetFromCache(Arguments args) { i -= 2) { o = cache->get(i); if (o == key) { - cache->set(JSFunctionResultCache::kFingerIndex, Smi::FromInt(i)); + cache->set_finger_index(i); return cache->get(i + 1); } } - const int size = - Smi::cast(cache->get(JSFunctionResultCache::kCacheSizeIndex))->value(); + int size = cache->size(); ASSERT(size <= cache->length()); for (int i = size - 2; i > finger_index; i -= 2) { o = cache->get(i); if (o == key) { - cache->set(JSFunctionResultCache::kFingerIndex, Smi::FromInt(i)); + cache->set_finger_index(i); return cache->get(i + 1); } } - // Cache miss. If we have spare room, put new data into it, otherwise - // evict post finger entry which must be least recently used. - if (size < cache->length()) { - cache->set(JSFunctionResultCache::kCacheSizeIndex, Smi::FromInt(size + 2)); - return CacheMiss(cache, size, key); + // There is no value in the cache. Invoke the function and cache result. + HandleScope scope; + + Handle cache_handle(cache); + Handle key_handle(key); + Handle value; + { + Handle factory(JSFunction::cast( + cache_handle->get(JSFunctionResultCache::kFactoryIndex))); + // TODO(antonm): consider passing a receiver when constructing a cache. + Handle receiver(Top::global_context()->global()); + // This handle is nor shared, nor used later, so it's safe. + Object** argv[] = { key_handle.location() }; + bool pending_exception = false; + value = Execution::Call(factory, + receiver, + 1, + argv, + &pending_exception); + if (pending_exception) return Failure::Exception(); + } + +#ifdef DEBUG + cache_handle->JSFunctionResultCacheVerify(); +#endif + + // Function invocation may have cleared the cache. Reread all the data. + finger_index = cache_handle->finger_index(); + size = cache_handle->size(); + + // If we have spare room, put new data into it, otherwise evict post finger + // entry which is likely to be the least recently used. + int index = -1; + if (size < cache_handle->length()) { + cache_handle->set_size(size + JSFunctionResultCache::kEntrySize); + index = size; } else { - int target_index = finger_index + JSFunctionResultCache::kEntrySize; - if (target_index == cache->length()) { - target_index = JSFunctionResultCache::kEntriesIndex; + index = finger_index + JSFunctionResultCache::kEntrySize; + if (index == cache_handle->length()) { + index = JSFunctionResultCache::kEntriesIndex; } - return CacheMiss(cache, target_index, key); } + + ASSERT(index % 2 == 0); + ASSERT(index >= JSFunctionResultCache::kEntriesIndex); + ASSERT(index < cache_handle->length()); + + cache_handle->set(index, *key_handle); + cache_handle->set(index + 1, *value); + cache_handle->set_finger_index(index); + +#ifdef DEBUG + cache_handle->JSFunctionResultCacheVerify(); +#endif + + return *value; } #ifdef DEBUG -- 2.7.4