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

Make sure that allocations through CALL_HEAP_FUNCTION
and runtime calls from JavaScript will always succeed
eventually if we have enough memory.

Committed: http://code.google.com/p/v8/source/detail?r=646

[iposva, 2008-10-30 05:29:01]

I think this change is headed in the right direction overall. The devil will be
in trying to test this for all scenarios.

-Ivan

http://codereview.chromium.org/8700/diff/1/5
File src/heap-inl.h (right):

http://codereview.chromium.org/8700/diff/1/5#newcode60
Line 60: if (always_allocate_scope_depth_ == 0) return result;
Can't you use an accessor here which abstracts the ==0 knowledge? For example
you could use !always_allocate().

http://codereview.chromium.org/8700/diff/1/5#newcode212
Line 212: #define CALL_AND_RETRY(FUNCTION_CALL, RETURN_VALUE, RETURN_EMPTY) \
Oh my! Unfortunately I do agree that we have to do it this way. I also think
that calling Heap::CollectAllGarbage() is necessary. Incrementally collecting
just the space that is needed, could potentially stall you in a endless loop.

http://codereview.chromium.org/8700/diff/1/5#newcode234
Line 234: AlwaysAllocateScope __scope__; \
In my opinion adding parens, such as __scope__() signals to the reader that
there is an essential constructor associated with the variable.

http://codereview.chromium.org/8700/diff/1/5#newcode243
Line 243: return RETURN_EMPTY; \
Style question: Personally I prefer the aligned \ at the 80th column, but I do
not remember whether or not Google style has a rule for this.

http://codereview.chromium.org/8700/diff/1/4
File src/heap.cc (right):

http://codereview.chromium.org/8700/diff/1/4#newcode1046
Line 1046: // XXX ...
Spurious comment?

http://codereview.chromium.org/8700/diff/1/4#newcode1798
Line 1798: // TODO: ...
Spurious comment or a real todo outstanding?

http://codereview.chromium.org/8700/diff/1/4#newcode2124
Line 2124: // TODO: ...
Looks like you need to handle the allocation failure here, because you are not
going to automatically try to allocate in the alternative space when reentering.
That is the big difference to AllocateEmptyFixedArray above. Should maybe
comment better above.

Also everywhere where we allocate directly out of new_space_ we need to make
sure that we use an alternative space in the presence of allocation failure in
new gen and forced allocate.

http://codereview.chromium.org/8700/diff/1/12
File src/heap.h (right):

http://codereview.chromium.org/8700/diff/1/12#newcode941
Line 941: class AlwaysAllocateScope {
What bothers me about this name that scope is a very overloaded term in the V8
context (another overloaded term!)

http://codereview.chromium.org/8700/diff/1/12#newcode944
Line 944: ASSERT(Heap::always_allocate_scope_depth_ == 0);
This means that you cannot nest these AlwaysAllocateScopes. Is that the
intention?
Can you really enforce this? What if you have to call out of C++ to JS as part
of the operation and end up calling back into the runtime?

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

LGTM

http://codereview.chromium.org/8700/diff/204/208
File src/heap-inl.h (right):

http://codereview.chromium.org/8700/diff/204/208#newcode60
Line 60: if (!always_allocate() || !result->IsFailure()) return result;
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;
}

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

LGTM.

Apart from these there are 3 places we need to check, perhaps in a new change. 
Search for IsRetryAfterGC in jsregexp.cc, factory.cc and builtins.cc.

http://codereview.chromium.org/8700/diff/204/208
File src/heap-inl.h (right):

http://codereview.chromium.org/8700/diff/204/208#newcode43
Line 43: AllocationSpace retry_space) {
This is fine for now.  In the long run I think we should just always use
OLD_POINTER_SPACE for the retry_space.

http://codereview.chromium.org/8700/diff/204/208#newcode210
Line 210: // Warning: Do not use the identifiers __object__ or __scope__ in a
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.

http://codereview.chromium.org/8700/diff/204/208#newcode219
Line 219: v8::internal::V8::FatalProcessOutOfMemory("CALL_AND_RETRY_0");      \
Is there a subtle fall-through here?  Comment needed?

http://codereview.chromium.org/8700/diff/204/207
File src/heap.cc (right):

http://codereview.chromium.org/8700/diff/204/207#newcode1041
Line 1041: if (always_allocate()) return AllocateHeapNumber(value, NOT_TENURED);
I wonder whether this will be a performance hit.  I'd prefer to move the
always_allocate test down to the body of the if below.

http://codereview.chromium.org/8700/diff/204/215
File src/heap.h (right):

http://codereview.chromium.org/8700/diff/204/215#newcode263
Line 263: static bool always_allocate() { return always_allocate_scope_depth_ !=
0; }
As discussed offline the next change should have a command line flag to make
this always return true.  At least in the DEBUG version.

[bak, 2008-10-30 11:17:02]

LGTM,
  Lars

http://codereview.chromium.org/8700/diff/204/211
File src/spaces.cc (right):

http://codereview.chromium.org/8700/diff/204/211#newcode1536
Line 1536: if (!Heap::always_allocate() &&
Heap::OldGenerationAllocationLimitReached()) {
This expression is used several times. Please make an abstraction.

[Kevin Millikin (Chromium), 2012-05-07 09:29:08]

http://codereview.chromium.org/8700/diff/204/test/cctest/test-alloc.cc
File test/cctest/test-alloc.cc (right):

http://codereview.chromium.org/8700/diff/204/test/cctest/test-alloc.cc#newcode56
test/cctest/test-alloc.cc:56: + 
CHECK(!Heap::AllocateHeapNumber(0.42)->IsFailure());
If this is meant to test the retry space for heap numbers, then we should assert
that heap number size is >= 0-element byte array (or >= new space available) to
make sure we're not hitting a hole at the top of the new space.