As discussed on the phone, I'd like your thoughts on the...

src/heap-inl.h

 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 21 matching lines @@
 #include "v8-counters.h"
 
 namespace v8 { namespace internal {
 
 int Heap::MaxHeapObjectSize() {
   return Page::kMaxHeapObjectSize;
 }
 
 
 Object* Heap::AllocateRaw(int size_in_bytes,
-                          AllocationSpace space) {
+                          AllocationSpace space,
+                          AllocationSpace retry_space) {

[Erik Corry, 2008/10/30 09:08:43]

This is fine for now.  In the long run I think we should just always use
OLD_POINTER_SPACE for the retry_space.

   ASSERT(allocation_allowed_ && gc_state_ == NOT_IN_GC);
+  ASSERT(space != NEW_SPACE ||
+         retry_space == OLD_POINTER_SPACE ||
+         retry_space == OLD_DATA_SPACE);
 #ifdef DEBUG
   if (FLAG_gc_interval >= 0 &&
       !disallow_allocation_failure_ &&
       Heap::allocation_timeout_-- <= 0) {
     return Failure::RetryAfterGC(size_in_bytes, space);
   }
   Counters::objs_since_last_full.Increment();
   Counters::objs_since_last_young.Increment();
 #endif
+  Object* result;
   if (NEW_SPACE == space) {
-    return new_space_.AllocateRaw(size_in_bytes);
+    result = new_space_.AllocateRaw(size_in_bytes);
+    if (!always_allocate() || !result->IsFailure()) return result;

[Mads Ager (chromium), 2008/10/30 08:48:14]

Normally, I like these bail-outs instead of if-else.  However, in this case
because of the negations in the condition, I think I would prefer:

if (result->IsFailure() && always_allocate()) {
  space = retry_space;
} else {
  return result;
}

+    space = retry_space;
   }
 
-  Object* result;
   if (OLD_POINTER_SPACE == space) {
     result = old_pointer_space_->AllocateRaw(size_in_bytes);
   } else if (OLD_DATA_SPACE == space) {
     result = old_data_space_->AllocateRaw(size_in_bytes);
   } else if (CODE_SPACE == space) {
     result = code_space_->AllocateRaw(size_in_bytes);
   } else if (LO_SPACE == space) {
     result = lo_space_->AllocateRaw(size_in_bytes);
   } else {
     ASSERT(MAP_SPACE == space);
@@ skipping 57 matching lines @@
 
 void Heap::RecordWrite(Address address, int offset) {
   if (new_space_.Contains(address)) return;
   ASSERT(!new_space_.FromSpaceContains(address));
   SLOW_ASSERT(Contains(address + offset));
   Page::SetRSet(address, offset);
 }
 
 
 OldSpace* Heap::TargetSpace(HeapObject* object) {
+  InstanceType type = object->map()->instance_type();
+  AllocationSpace space = TargetSpaceId(type);
+  return (space == OLD_POINTER_SPACE)
+      ? old_pointer_space_
+      : old_data_space_;
+}
+
+
+AllocationSpace Heap::TargetSpaceId(InstanceType type) {
   // Heap numbers and sequential strings are promoted to old data space, all
   // other object types are promoted to old pointer space.  We do not use
   // object->IsHeapNumber() and object->IsSeqString() because we already
   // know that object has the heap object tag.
-  InstanceType type = object->map()->instance_type();
   ASSERT((type != CODE_TYPE) && (type != MAP_TYPE));
   bool has_pointers =
       type != HEAP_NUMBER_TYPE &&
       (type >= FIRST_NONSTRING_TYPE ||
-       String::cast(object)->representation_tag() != kSeqStringTag);
-  return has_pointers ? old_pointer_space_ : old_data_space_;
+       (type & kStringRepresentationMask) != kSeqStringTag);
+  return has_pointers ? OLD_POINTER_SPACE : OLD_DATA_SPACE;
 }
 
 
 void Heap::CopyBlock(Object** dst, Object** src, int byte_size) {
   ASSERT(IsAligned(byte_size, kPointerSize));
 
   // Use block copying memcpy if the segment we're copying is
   // enough to justify the extra call/setup overhead.
   static const int kBlockCopyLimit = 16 * kPointerSize;
 
@@ skipping 25 matching lines @@
 
 
 void Heap::ClearKeyedLookupCache() {
   keyed_lookup_cache_ = undefined_value();
 }
 
 
 #define GC_GREEDY_CHECK() \
   ASSERT(!FLAG_gc_greedy || v8::internal::Heap::GarbageCollectionGreedyCheck())
 
-// Do not use the identifier __object__ in a call to this macro.
-//
-// Call the function FUNCTION_CALL.  If it fails with a RetryAfterGC
-// failure, call the garbage collector and retry the function.  If the
-// garbage collector cannot reclaim the required space or the second
-// call fails with a RetryAfterGC failure, fail with out of memory.
-// If there is any other failure, return a null handle.  If either
-// call succeeds, return a handle to the functions return value.
-//
-// Note that this macro always returns or raises a fatal error.
-#define CALL_HEAP_FUNCTION(FUNCTION_CALL, TYPE)                              \
-  do {                                                                       \
-    GC_GREEDY_CHECK();                                                       \
-    Object* __object__ = FUNCTION_CALL;                                      \
-    if (__object__->IsFailure()) {                                           \
-      if (__object__->IsRetryAfterGC()) {                                    \
-        if (!Heap::CollectGarbage(                                           \
-                Failure::cast(__object__)->requested(),                      \
-                Failure::cast(__object__)->allocation_space())) {            \
-          /* TODO(1181417): Fix this. */                                     \
-          v8::internal::V8::FatalProcessOutOfMemory("CALL_HEAP_FUNCTION");   \
-        }                                                                    \
-        __object__ = FUNCTION_CALL;                                          \
-        if (__object__->IsFailure()) {                                       \
-          if (__object__->IsRetryAfterGC()) {                                \
-            /* TODO(1181417): Fix this. */                                   \
-            v8::internal::V8::FatalProcessOutOfMemory("CALL_HEAP_FUNCTION"); \
-          }                                                                  \
-          return Handle<TYPE>();                                             \
-        }                                                                    \
-      } else {                                                               \
-        if (__object__->IsOutOfMemoryFailure()) {                            \
-          v8::internal::V8::FatalProcessOutOfMemory("CALL_HEAP_FUNCTION");   \
-        }                                                                    \
-        return Handle<TYPE>();                                               \
-      }                                                                      \
-    }                                                                        \
-    return Handle<TYPE>(TYPE::cast(__object__));                             \
+
+// Calls the FUNCTION_CALL function and retries it up to three times
+// to guarantee that any allocations performed during the call will
+// succeed if there's enough memory.
+
+// Warning: Do not use the identifiers __object__ or __scope__ in a

[Erik Corry, 2008/10/30 09:08:43]

I don't really feel we need this warning, but if we have it we should have it on
CALL_HEAP_FUNCTION and CALL_HEAP_FUNCTION_VOID, which are the macros used by
others.

+// call to this macro.
+
+#define CALL_AND_RETRY(FUNCTION_CALL, RETURN_VALUE, RETURN_EMPTY)         \
+  do {                                                                    \
+    GC_GREEDY_CHECK();                                                    \
+    Object* __object__ = FUNCTION_CALL;                                   \
+    if (!__object__->IsFailure()) return RETURN_VALUE;                    \
+    if (__object__->IsOutOfMemoryFailure()) {                             \
+      v8::internal::V8::FatalProcessOutOfMemory("CALL_AND_RETRY_0");      \

[Erik Corry, 2008/10/30 09:08:43]

Is there a subtle fall-through here?  Comment needed?

+    }                                                                     \
+    if (!__object__->IsRetryAfterGC()) return RETURN_EMPTY;               \
+    if (!Heap::CollectGarbage(                                            \
+            Failure::cast(__object__)->requested(),                       \
+            Failure::cast(__object__)->allocation_space())) {             \
+      v8::internal::V8::FatalProcessOutOfMemory("CALL_AND_RETRY_1");      \
+    }                                                                     \
+    __object__ = FUNCTION_CALL;                                           \
+    if (!__object__->IsFailure()) return RETURN_VALUE;                    \
+    if (__object__->IsOutOfMemoryFailure()) {                             \
+      v8::internal::V8::FatalProcessOutOfMemory("CALL_AND_RETRY_2");      \
+    }                                                                     \
+    if (!__object__->IsRetryAfterGC()) return RETURN_EMPTY;               \
+    Counters::gc_last_resort_from_handles.Increment();                    \
+    Heap::CollectAllGarbage();                                            \
+    {                                                                     \
+      AlwaysAllocateScope __scope__;                                      \
+      __object__ = FUNCTION_CALL;                                         \
+    }                                                                     \
+    if (!__object__->IsFailure()) return RETURN_VALUE;                    \
+    if (__object__->IsOutOfMemoryFailure()) {                             \
+      /* TODO(1181417): Fix this. */                                      \
+      v8::internal::V8::FatalProcessOutOfMemory("CALL_AND_RETRY_3");      \
+    }                                                                     \
+    ASSERT(!__object__->IsRetryAfterGC());                                \
+    return RETURN_EMPTY;                                                  \
   } while (false)
 
 
-// Don't use the following names: __object__, __failure__.
-#define CALL_HEAP_FUNCTION_VOID(FUNCTION_CALL)                      \
-  GC_GREEDY_CHECK();                                                \
-  Object* __object__ = FUNCTION_CALL;                               \
-  if (__object__->IsFailure()) {                                    \
-    if (__object__->IsRetryAfterGC()) {                             \
-      Failure* __failure__ = Failure::cast(__object__);             \
-      if (!Heap::CollectGarbage(__failure__->requested(),           \
-                                __failure__->allocation_space())) { \
-         /* TODO(1181417): Fix this. */                             \
-         V8::FatalProcessOutOfMemory("Handles");                    \
-      }                                                             \
-      __object__ = FUNCTION_CALL;                                   \
-      if (__object__->IsFailure()) {                                \
-        if (__object__->IsRetryAfterGC()) {                         \
-           /* TODO(1181417): Fix this. */                           \
-           V8::FatalProcessOutOfMemory("Handles");                  \
-        }                                                           \
-        return;                                                     \
-      }                                                             \
-    } else {                                                        \
-      if (__object__->IsOutOfMemoryFailure()) {                     \
-         V8::FatalProcessOutOfMemory("Handles");                    \
-      }                                                             \
-      UNREACHABLE();                                                \
-    }                                                               \
-  }
+#define CALL_HEAP_FUNCTION(FUNCTION_CALL, TYPE)         \
+  CALL_AND_RETRY(FUNCTION_CALL,                         \
+                 Handle<TYPE>(TYPE::cast(__object__)),  \
+                 Handle<TYPE>())
+
+
+#define CALL_HEAP_FUNCTION_VOID(FUNCTION_CALL) \
+  CALL_AND_RETRY(FUNCTION_CALL,,)
 
 
 #ifdef DEBUG
 
 inline bool Heap::allow_allocation(bool new_state) {
   bool old = allocation_allowed_;
   allocation_allowed_ = new_state;
   return old;
 }
 
 #endif
 
 
 } }  // namespace v8::internal
 
 #endif  // V8_HEAP_INL_H_