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

Enable physical memory argument to be passed as an argument to ConfigureResourceConstraintsForPlatform.

BUG=312241
R=svenpanne@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=17696

[rmcilroy, 2013-11-11 11:21:57]

PTAL Sven.

[Sven Panne, 2013-11-12 07:45:02]

First round of comments.

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc
File src/defaults.cc (right):

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc#newcode40
src/defaults.cc:40: #if V8_OS_ANDROID
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".

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc#newcode49
src/defaults.cc:49: if (constraints == NULL) {
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".

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc#newcode53
src/defaults.cc:53: int lump_of_memory = (i::kPointerSize / 4) * i::MB;
const

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

Thanks Sven. PTAL.

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc
File src/defaults.cc (right):

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc#newcode40
src/defaults.cc:40: #if V8_OS_ANDROID
On 2013/11/12 07:45:03, Sven Panne wrote:
> 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".

Done (defined three names for consistency).

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc#newcode49
src/defaults.cc:49: if (constraints == NULL) {
On 2013/11/12 07:45:03, Sven Panne wrote:
> 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".

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?

https://codereview.chromium.org/68203003/diff/30001/src/defaults.cc#newcode53
src/defaults.cc:53: int lump_of_memory = (i::kPointerSize / 4) * i::MB;
On 2013/11/12 07:45:03, Sven Panne wrote:
> const

Done.

[Sven Panne, 2013-11-12 14:05:21]

lgtm

[rmcilroy, 2013-11-12 14:44:38]

On 2013/11/12 14:05:21, Sven Panne wrote:
> lgtm

Thanks Sven.  Shall I move ResourceConstraints::ConfigureDefaults to api.cc or
leave it in defaults.cc (personally I think it would be better in api.cc along
with other ResourceConstraints methods, but happy to keep it as is).

[Sven Panne, 2013-11-13 08:08:53]

On 2013/11/12 14:44:38, rmcilroy wrote:
> On 2013/11/12 14:05:21, Sven Panne wrote:
> > lgtm
> 
> Thanks Sven.  Shall I move ResourceConstraints::ConfigureDefaults to api.cc or
> leave it in defaults.cc (personally I think it would be better in api.cc along
> with other ResourceConstraints methods, but happy to keep it as is).

I would prefer api.cc, too. After Chrome is adapted, src/defaults.cc and
include/v8-defaults.h can die..

[rmcilroy, 2013-11-13 10:39:23]

On 2013/11/13 08:08:53, Sven Panne wrote:
> On 2013/11/12 14:44:38, rmcilroy wrote:
> > On 2013/11/12 14:05:21, Sven Panne wrote:
> > > lgtm
> > 
> > Thanks Sven.  Shall I move ResourceConstraints::ConfigureDefaults to api.cc
or
> > leave it in defaults.cc (personally I think it would be better in api.cc
along
> > with other ResourceConstraints methods, but happy to keep it as is).
> 
> I would prefer api.cc, too. After Chrome is adapted, src/defaults.cc and
> include/v8-defaults.h can die..

SGTM, done.

[Sven Panne, 2013-11-13 12:10:31]

LGTM (still)

[rmcilroy, 2013-11-13 14:05:14]

Committed patchset #5 manually as r17696 (presubmit successful).