Add methods to enable configuration of ResourceConstraints based on limits derived at runtime.

src/heap.cc

Index: src/heap.cc
diff --git a/src/heap.cc b/src/heap.cc
index 23b7c89c4eace42a55d398bdf6ee9e68dbeab15d..f5f5d04602df5253e03fd802b8abdef74642505e 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -65,31 +65,36 @@ namespace v8 {
 namespace internal {
 
 
-Heap::Heap()
-    : isolate_(NULL),
 // semispace_size_ should be a power of 2 and old_generation_size_ should be
 // a multiple of Page::kPageSize.
 #if V8_TARGET_ARCH_X64
 #define LUMP_OF_MEMORY (2 * MB)
-      code_range_size_(512*MB),
 #else
 #define LUMP_OF_MEMORY MB
-      code_range_size_(0),
 #endif
 #if defined(ANDROID) || V8_TARGET_ARCH_MIPS

[Hannes Payer (out of office), 2013/09/19 14:50:57]

I never liked the ifdef here. Now it's time to get rid of it while you are
there. It would be great if the embedder (chrome, d8) would set the right
values. It also shouldn't be a binary decision. We have beefier and weaker
phones. Their could be arbitrary size values, depending on the device. WDYT?

[rmcilroy, 2013/09/19 15:01:34]

I don't like this ifdef particularly either, but I don't think forcing the
embedder to do this is the right way to do this.  The embedder doesn't
necessarily know what a good set of defaults are for different devices, and it
would just end up moving a single set of per-arch constants to sets of constants
in every single embedder, which are then just going to diverge and cause
unexpected performance differences as they diverge.  If the embedder really
knows what it wants these to be it can still override them using SetResources,
but I think it's good to have sensible defaults in here.

[Hannes Payer (out of office), 2013/09/19 16:25:28]

Well, what I mean is that we should have these ifdefs in chromium and in d8.
Right now we have Android/MIPS or others, X64 or others, and on top of that
resource constrained. We should keep X64 ifdef since this one is a true global
mode for v8. But apart from that we have 3 classes of devices right now:
- large memory device
- regular memory device
- low memory device
It would be actually awesome if the embedder would set arbitrary memory sizes,
based on the system state. But this is future work.

[rmcilroy, 2013/09/19 19:46:11]

There are more V8 embedders than just d8 and chrome, and I really don't agree
that it's cleaner to move from having one ifdef here to multiple ifdefs across
all the embedders.  Embedders shouldn't be forced to know what a reasonable size
for the semispace/oldspace/etc is or what the relationship between them is.

I do agree that we should reach a state where the embedder gives us a value for
how much memory is available (whether that's a signal for this being a
large/medium/low mem device, or is the actual available memory), and then V8
decides how to size these spaces based on this, but that's future work because
(i) currently Chrome doesn't even call SetResourceConstraints on V8 for the main
thread (I have a CL in the work to change this for the is_memory_constrained
flag), and (ii) due to the default isolate there is no particularly clear point
for the embedder to actually send this value and ensure that the heap is
configured correctly before it is setup.

-      reserved_semispace_size_(4 * Max(LUMP_OF_MEMORY, Page::kPageSize)),
-      max_semispace_size_(4 * Max(LUMP_OF_MEMORY, Page::kPageSize)),
-      initial_semispace_size_(Page::kPageSize),
-      max_old_generation_size_(192*MB),
-      max_executable_size_(max_old_generation_size_),
+#define DEFAULT_SEMISPACE_MAX_SIZE (4 * Max(LUMP_OF_MEMORY, Page::kPageSize))
+#define DEFAULT_OLD_GENERATION_MAX_SIZE (192*MB)
+#define DEFAULT_EXECUTABLE_MAX_SIZE (192*MB)
 #else
-      reserved_semispace_size_(8 * Max(LUMP_OF_MEMORY, Page::kPageSize)),
-      max_semispace_size_(8 * Max(LUMP_OF_MEMORY, Page::kPageSize)),
-      initial_semispace_size_(Page::kPageSize),
-      max_old_generation_size_(700ul * LUMP_OF_MEMORY),
-      max_executable_size_(256l * LUMP_OF_MEMORY),
+#define DEFAULT_SEMISPACE_MAX_SIZE (8 * Max(LUMP_OF_MEMORY, Page::kPageSize))
+#define DEFAULT_OLD_GENERATION_MAX_SIZE (700ul * LUMP_OF_MEMORY)
+#define DEFAULT_EXECUTABLE_MAX_SIZE (256l * LUMP_OF_MEMORY)
 #endif
 
+
+Heap::Heap()
+    : isolate_(NULL),
+#if V8_TARGET_ARCH_X64
+      code_range_size_(512*MB),
+#else
+      code_range_size_(0),
+#endif
+      reserved_semispace_size_(DEFAULT_SEMISPACE_MAX_SIZE),

[Hannes Payer (out of office), 2013/09/19 14:50:57]

Here we could set the values to proper default values, like our default values
for non-mobile devices now.

[rmcilroy, 2013/09/19 15:01:34]

I'm not sure what you mean here?

[Hannes Payer (out of office), 2013/09/19 16:25:28]

What I meant is to set the values to
reserved_semispace_size_(8 * Max(LUMP_OF_MEMORY, Page::kPageSize)),	
max_semispace_size_(8 * Max(LUMP_OF_MEMORY, Page::kPageSize)),	
initial_semispace_size_(Page::kPageSize),	
max_old_generation_size_(700ul * LUMP_OF_MEMORY),	
max_executable_size_(256l * LUMP_OF_MEMORY),

[rmcilroy, 2013/09/19 19:46:11]

Maybe we could just set these all to zero initially, and do the default setting
in ConfigureHeap (which should be called from ConfigureDefaultHeap even if the
embedder doesn't call SetResourceConstraints). WDYT?

+      max_semispace_size_(DEFAULT_SEMISPACE_MAX_SIZE),
+      initial_semispace_size_(Page::kPageSize),
+      max_old_generation_size_(DEFAULT_OLD_GENERATION_MAX_SIZE),
+      max_executable_size_(DEFAULT_EXECUTABLE_MAX_SIZE),
 // Variables set based on semispace_size_ and old_generation_size_ in
 // ConfigureHeap (survived_since_last_expansion_, external_allocation_limit_)
 // Will be 4 * reserved_semispace_size_ to ensure that young
@@ -161,6 +166,7 @@ Heap::Heap()
 #endif
       promotion_queue_(this),
       configured_(false),
+      limits_changed_from_defaults_(false),
       chunks_queued_for_free_(NULL),
       relocation_mutex_(NULL) {
   // Allow build-time customization of the max semispace size. Building
@@ -169,7 +175,6 @@ Heap::Heap()
 #if defined(V8_MAX_SEMISPACE_SIZE)
   max_semispace_size_ = reserved_semispace_size_ = V8_MAX_SEMISPACE_SIZE;
 #endif

[Hannes Payer (out of office), 2013/09/19 14:50:57]

Can you bring back this newline?

-
   intptr_t max_virtual = OS::MaxVirtualMemory();
 
   if (max_virtual > 0) {
@@ -6673,6 +6678,16 @@ bool Heap::ConfigureHeap(int max_semispace_size,
                          intptr_t max_executable_size) {
   if (HasBeenSetUp()) return false;
 
+  if (isolate_->is_memory_constrained() && !limits_changed_from_defaults_) {
+    // Update the default values if this is a memory constrained device.
+    // TODO(rmcilroy): Ideally these should be set correctly in the constructor
+    // based on this flag, but the flag isn't setup correctly at that point.
+    max_semispace_size_ = DEFAULT_SEMISPACE_MAX_SIZE / 4;
+    reserved_semispace_size_ = DEFAULT_SEMISPACE_MAX_SIZE / 4;
+    max_old_generation_size_ = DEFAULT_OLD_GENERATION_MAX_SIZE / 2;
+    max_executable_size_ = DEFAULT_EXECUTABLE_MAX_SIZE / 2;

[Hannes Payer (out of office), 2013/09/19 14:50:57]

The embedder would just set the right values, no memory_constrained guard
needed.

[rmcilroy, 2013/09/19 15:01:34]

The embedder doesn't need to set these values (if they are 0 then no changes are
made in this method). This branch is simply to set the initial set of default
values correctly based on the flag (see the comment above - this should really
be alongside the defaults set in the constructor, but can't due to the fact the
flags are set late). 

[Hannes Payer (out of office), 2013/09/19 16:25:28]

But SetResourceConstraints could just set it to the right values. That would
work, right?

[rmcilroy, 2013/09/19 19:46:11]

Do you mean the embedder could just pass the correct values to set?  If so, see
my comment above about not thinking we should require the embedder to know how
to set semispace/oldspace/etc. (they should only setting these values if they
really know what they are doing, otherwise we should do a good default IMO).

+  }
+
   if (FLAG_stress_compaction) {
     // This will cause more frequent GCs when stressing.
     max_semispace_size_ = Page::kPageSize;
@@ -6713,6 +6728,12 @@ bool Heap::ConfigureHeap(int max_semispace_size,
     max_executable_size_ = RoundUp(max_executable_size, Page::kPageSize);
   }
 
+  if (max_semispace_size > 0 || max_old_gen_size > 0 ||
+      max_executable_size_ > 0) {
+    // Did we change anything from their defaults.
+    limits_changed_from_defaults_ = true;
+  }
+
   // The max executable size must be less than or equal to the max old
   // generation size.
   if (max_executable_size_ > max_old_generation_size_) {