From c65edf93b1796893c4cea444921494fcf133b7af Mon Sep 17 00:00:00 2001 From: "mvstanton@chromium.org" Date: Mon, 3 Nov 2014 08:43:20 +0000 Subject: [PATCH] Fix for bug 429168, PdfJs regression. We pay a very high cost for AllocationResult being a > kPointerSize struct. This can be avoided by using Smis to indicate failure with retry spaces. BUG=429168 LOG=N R=yangguo@chromium.org Review URL: https://codereview.chromium.org/699473002 Cr-Commit-Position: refs/heads/master@{#25057} git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@25057 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/globals.h | 1 - src/heap/heap-inl.h | 2 -- src/heap/heap.cc | 2 -- src/heap/spaces.h | 19 ++++++++++++------- src/serialize.cc | 2 +- src/serialize.h | 2 +- test/cctest/test-alloc.cc | 6 +++--- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/globals.h b/src/globals.h index 03eb99e15d..4ea06b2d04 100644 --- a/src/globals.h +++ b/src/globals.h @@ -376,7 +376,6 @@ enum AllocationSpace { CELL_SPACE, // Only and all cell objects. PROPERTY_CELL_SPACE, // Only and all global property cell objects. LO_SPACE, // Promoted large objects. - INVALID_SPACE, // Only used in AllocationResult to signal success. FIRST_SPACE = NEW_SPACE, LAST_SPACE = LO_SPACE, diff --git a/src/heap/heap-inl.h b/src/heap/heap-inl.h index 9e3421e322..48e928d711 100644 --- a/src/heap/heap-inl.h +++ b/src/heap/heap-inl.h @@ -458,8 +458,6 @@ bool Heap::AllowedToBeMigrated(HeapObject* obj, AllocationSpace dst) { case PROPERTY_CELL_SPACE: case LO_SPACE: return false; - case INVALID_SPACE: - break; } UNREACHABLE(); return false; diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 93e00d2d0b..5c8cd4528a 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -4545,8 +4545,6 @@ bool Heap::InSpace(Address addr, AllocationSpace space) { return property_cell_space_->Contains(addr); case LO_SPACE: return lo_space_->SlowContains(addr); - case INVALID_SPACE: - break; } UNREACHABLE(); return false; diff --git a/src/heap/spaces.h b/src/heap/spaces.h index 1944464a7c..ef294b2439 100644 --- a/src/heap/spaces.h +++ b/src/heap/spaces.h @@ -1614,16 +1614,19 @@ class AllocationResult { public: // Implicit constructor from Object*. AllocationResult(Object* object) // NOLINT - : object_(object), - retry_space_(INVALID_SPACE) {} + : object_(object) { + // AllocationResults can't return Smis, which are used to represent + // failure and the space to retry in. + CHECK(!object->IsSmi()); + } - AllocationResult() : object_(NULL), retry_space_(INVALID_SPACE) {} + AllocationResult() : object_(Smi::FromInt(NEW_SPACE)) {} static inline AllocationResult Retry(AllocationSpace space = NEW_SPACE) { return AllocationResult(space); } - inline bool IsRetry() { return retry_space_ != INVALID_SPACE; } + inline bool IsRetry() { return object_->IsSmi(); } template bool To(T** obj) { @@ -1639,18 +1642,20 @@ class AllocationResult { AllocationSpace RetrySpace() { DCHECK(IsRetry()); - return retry_space_; + return static_cast(Smi::cast(object_)->value()); } private: explicit AllocationResult(AllocationSpace space) - : object_(NULL), retry_space_(space) {} + : object_(Smi::FromInt(static_cast(space))) {} Object* object_; - AllocationSpace retry_space_; }; +STATIC_ASSERT(sizeof(AllocationResult) == kPointerSize); + + class PagedSpace : public Space { public: // Creates a space with a maximum capacity, and an id. diff --git a/src/serialize.cc b/src/serialize.cc index df62adba76..6347943067 100644 --- a/src/serialize.cc +++ b/src/serialize.cc @@ -1903,7 +1903,7 @@ AllocationSpace Serializer::SpaceOfObject(HeapObject* object) { } } UNREACHABLE(); - return INVALID_SPACE; + return FIRST_SPACE; } diff --git a/src/serialize.h b/src/serialize.h index 9c561187bf..d7a6c7fa30 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -296,7 +296,7 @@ class SerializerDeserializer: public ObjectVisitor { // No reservation for large object space necessary. static const int kNumberOfPreallocatedSpaces = LO_SPACE; - static const int kNumberOfSpaces = INVALID_SPACE; + static const int kNumberOfSpaces = LAST_SPACE + 1; protected: // Where the pointed-to object can be found: diff --git a/test/cctest/test-alloc.cc b/test/cctest/test-alloc.cc index d647a3128e..54d516e13e 100644 --- a/test/cctest/test-alloc.cc +++ b/test/cctest/test-alloc.cc @@ -86,7 +86,7 @@ static AllocationResult AllocateAfterFailures() { Builtins::kIllegal)).ToObjectChecked(); // Return success. - return Smi::FromInt(42); + return heap->true_value(); } @@ -100,7 +100,7 @@ TEST(StressHandles) { v8::Handle env = v8::Context::New(CcTest::isolate()); env->Enter(); Handle o = Test(); - CHECK(o->IsSmi() && Smi::cast(*o)->value() == 42); + CHECK(o->IsTrue()); env->Exit(); } @@ -162,7 +162,7 @@ TEST(StressJS) { // Call the accessor through JavaScript. v8::Handle result = v8::Script::Compile( v8::String::NewFromUtf8(CcTest::isolate(), "(new Foo).get"))->Run(); - CHECK_EQ(42, result->Int32Value()); + CHECK_EQ(true, result->BooleanValue()); env->Exit(); }