Enable physical memory argument to be passed as an argument to ConfigureResourceConstraintsForPlatf…

src/defaults.cc

Index: src/defaults.cc
diff --git a/src/defaults.cc b/src/defaults.cc
index a03cf69b088e4f7cfa0b65f5635edd43670f61e7..aee1a438f143578ece7afa23ceb56e1d73470e3b 100644
--- a/src/defaults.cc
+++ b/src/defaults.cc
@@ -37,6 +37,45 @@
 namespace v8 {
 
 
+#if V8_OS_ANDROID

[Sven Panne, 2013/11/12 07:45:03]

Move the #if into the function below to reduce its scope
(http://refactoring.com/catalog/reduceScopeOfVariable.html). Furthermore, I
think that directly defining 2 limits is much clearer than the slightly cryptic
kOsHasSwap:

#if V8_OS_ANDROID
const uint64_t limit1 = 768ul * i::MB;
const uint64_t limit2 = 1ul  * i::GB;
#else
...
#endif

Feel free to choose something better than the names "limit1" and "limit2".

[rmcilroy, 2013/11/12 12:13:58]

Done (defined three names for consistency).

+const bool kOsHasSwap = false;
+#else
+const bool kOsHasSwap = true;
+#endif
+
+
+bool ConfigureDefaultResourceConstraints(
+     ResourceConstraints* constraints, uint64_t physical_memory) {
+  if (constraints == NULL) {

[Sven Panne, 2013/11/12 07:45:03]

This looks strange and a bit ugly: Why do we need a NULL check here and what is
the return value used for? It looks like the caller shouldn't have called this
if there are no constraints.

I propose to return void and make the whole function a member function of
ResourceConstraints under a more suitable name like "ConfigureDefaults".

[rmcilroy, 2013/11/12 12:13:58]

I like the idea of making the function a member of ResourceConstraints, but
there was previously push-back on having this be part of v8.h. I've made the
change here but: (i) is there agreement now that this can be part of v8.h; and
(ii) should we add the implementation to api.cc and remove the whole defaults.cc
file in this case? 

+    return false;
+  }
+
+  int lump_of_memory = (i::kPointerSize / 4) * i::MB;

[Sven Panne, 2013/11/12 07:45:03]

const

[rmcilroy, 2013/11/12 12:13:58]

Done.

+
+  // The young_space_size should be a power of 2 and old_generation_size should
+  // be a multiple of Page::kPageSize.
+  if (physical_memory <= 512ul * i::MB) {
+    constraints->set_max_young_space_size(2 * lump_of_memory);
+    constraints->set_max_old_space_size(128 * lump_of_memory);
+    constraints->set_max_executable_size(96 * lump_of_memory);
+  } else if (physical_memory <= (kOsHasSwap ? 768ul * i::MB : 1ul * i::GB)) {
+    constraints->set_max_young_space_size(8 * lump_of_memory);
+    constraints->set_max_old_space_size(256 * lump_of_memory);
+    constraints->set_max_executable_size(192 * lump_of_memory);
+  } else if (physical_memory <= (kOsHasSwap ? 1ul * i::GB : 2ul * i::GB)) {
+    constraints->set_max_young_space_size(16 * lump_of_memory);
+    constraints->set_max_old_space_size(512 * lump_of_memory);
+    constraints->set_max_executable_size(256 * lump_of_memory);
+  } else {
+    constraints->set_max_young_space_size(16 * lump_of_memory);
+    constraints->set_max_old_space_size(700 * lump_of_memory);
+    constraints->set_max_executable_size(256 * lump_of_memory);
+  }
+  return true;
+}
+
+
+// TODO(rmcilroy): Remove this function once it is no longer used in Chrome.
 bool ConfigureResourceConstraintsForCurrentPlatform(
     ResourceConstraints* constraints) {
   if (constraints == NULL) {
@@ -60,6 +99,7 @@ bool ConfigureResourceConstraintsForCurrentPlatform(
 }
 
 
+// TODO(rmcilroy): Remove this function once it is no longer used in Chrome.
 bool SetDefaultResourceConstraintsForCurrentPlatform() {
   ResourceConstraints constraints;
   if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints))