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

src/defaults.cc

 // Copyright 2013 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 19 matching lines @@
 #undef USING_V8_SHARED
 #include "../include/v8-defaults.h"
 
 #include "platform.h"
 #include "globals.h"
 #include "v8.h"
 
 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) {
     return false;
   }
 
   int lump_of_memory = (i::kPointerSize / 4) * i::MB;
 
   // The young_space_size should be a power of 2 and old_generation_size should
   // be a multiple of Page::kPageSize.
 #if V8_OS_ANDROID
   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
   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);
 #endif
   return true;
 }
 
 
+// TODO(rmcilroy): Remove this function once it is no longer used in Chrome.
 bool SetDefaultResourceConstraintsForCurrentPlatform() {
   ResourceConstraints constraints;
   if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints))
     return false;
   return SetResourceConstraints(&constraints);
 }
 
 }  // namespace v8