Add 128MB limit for executable pages.

Add 128MB limit for executable pages.

BUG=http://code.google.com/p/v8/issues/detail?id=925
TEST=None.

[jschuh, 2010-11-05 00:06:42]

This is just an unrotted version of Paul's original patch that I walked through
and tested. I tried to follow the convention of int and intptr_t, but it appears
that is a bit in flux right now.

[Mads Ager (chromium), 2010-11-05 07:49:01]

http://codereview.chromium.org/4397004/diff/19001/20001
File include/v8.h (right):

http://codereview.chromium.org/4397004/diff/19001/20001#newcode2497
include/v8.h:2497: total_heap_size_executable_ = size; }
'}' on new line.

http://codereview.chromium.org/4397004/diff/19001/20003
File src/heap.cc (right):

http://codereview.chromium.org/4397004/diff/19001/20003#newcode92
src/heap.cc:92: intptr_t Heap::max_executable_size_ = 128*MB;
Do you want the limit to be the same on the 64-bit version?  Code objects are
usually quite a bit bigger in the 64-bit version. Do we have any data for
64-bit?

http://codereview.chromium.org/4397004/diff/19001/20005
File src/spaces.cc (right):

http://codereview.chromium.org/4397004/diff/19001/20005#newcode364
src/spaces.cc:364: if (executable == EXECUTABLE) {
We should be able to combine this check with the other executable check that we
perform already below. This adds an extra check for all allocations. We can make
it extra checks only for executable memory.

http://codereview.chromium.org/4397004/diff/19001/20005#newcode366
src/spaces.cc:366: static_cast<size_t>(capacity_executable_)) {
Indentation off.

http://codereview.chromium.org/4397004/diff/19001/20006
File src/spaces.h (right):

http://codereview.chromium.org/4397004/diff/19001/20006#newcode596
src/spaces.h:596: 0 : capacity_executable_ - size_executable_; }
Please move '}' to new line.

I would prefer:

if (capacity_executable_ < size_executable_) return 0;
return capacity_executable_ - size_executable_;

[jschuh, 2010-11-10 00:44:58]

Fixed the formatting, increased 64-bit limit to 256MB, and collapsed the three
executable checks into one.

Sorry about the formatting stuff, btw. I have no idea how I missed that in the
lint output.


On 2010/11/05 07:49:01, Mads Ager wrote:
> http://codereview.chromium.org/4397004/diff/19001/20001
> File include/v8.h (right):
> 
> http://codereview.chromium.org/4397004/diff/19001/20001#newcode2497
> include/v8.h:2497: total_heap_size_executable_ = size; }
> '}' on new line.
> 
> http://codereview.chromium.org/4397004/diff/19001/20003
> File src/heap.cc (right):
> 
> http://codereview.chromium.org/4397004/diff/19001/20003#newcode92
> src/heap.cc:92: intptr_t Heap::max_executable_size_ = 128*MB;
> Do you want the limit to be the same on the 64-bit version?  Code objects are
> usually quite a bit bigger in the 64-bit version. Do we have any data for
> 64-bit?
> 
> http://codereview.chromium.org/4397004/diff/19001/20005
> File src/spaces.cc (right):
> 
> http://codereview.chromium.org/4397004/diff/19001/20005#newcode364
> src/spaces.cc:364: if (executable == EXECUTABLE) {
> We should be able to combine this check with the other executable check that
we
> perform already below. This adds an extra check for all allocations. We can
make
> it extra checks only for executable memory.
> 
> http://codereview.chromium.org/4397004/diff/19001/20005#newcode366
> src/spaces.cc:366: static_cast<size_t>(capacity_executable_)) {
> Indentation off.
> 
> http://codereview.chromium.org/4397004/diff/19001/20006
> File src/spaces.h (right):
> 
> http://codereview.chromium.org/4397004/diff/19001/20006#newcode596
> src/spaces.h:596: 0 : capacity_executable_ - size_executable_; }
> Please move '}' to new line.
> 
> I would prefer:
> 
> if (capacity_executable_ < size_executable_) return 0;
> return capacity_executable_ - size_executable_;

[Mads Ager (chromium), 2010-11-10 08:14:24]

LGTM, I'll take care of these nits and land.

http://codereview.chromium.org/4397004/diff/42002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/4397004/diff/42002/src/heap.cc#newcode4348
src/heap.cc:4348: if (max_executable_size > 0)
Let's add braces here.

http://codereview.chromium.org/4397004/diff/42002/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/4397004/diff/42002/src/spaces.cc#newcode369
src/spaces.cc:369: "V8 Executable Allocation capacity exceeded"));
Indentation off by one space it seems?

[Mads Ager (chromium), 2010-11-10 08:23:26]

http://codereview.chromium.org/4397004/diff/42002/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/4397004/diff/42002/src/spaces.cc#newcode373
src/spaces.cc:373: size_executable_ += static_cast<int>(*allocated);
This only get's updated when CodeRange::exists().

I'll take care of that it.

[Mads Ager (chromium), 2010-11-10 08:32:29]

http://codereview.chromium.org/4397004/diff/42002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/4397004/diff/42002/src/heap.cc#newcode4367
src/heap.cc:4367: return ConfigureHeap(FLAG_max_new_space_size / 2,
FLAG_max_old_space_size,
Removing the units here and not changing flag-definitions is unfortunate. Also,
the max_executable_size flag does not exist.

[Mads Ager (chromium), 2010-11-10 08:39:37]

Issues addressed and landed in r5799.