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

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

Adds ConfigureResourceConstraintsForCurrentPlatform and SetDefaultResourceConstraintsForCurrentPlatform which configure the heap based on the available physical memory, rather than hard-coding by platform as previous.  This change also adds OS::TotalPhysicalMemory to platform.h.

BUG=292928
R=danno@chromium.org, hpayer@chromium.org

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

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

https://codereview.chromium.org/24269003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode75
src/heap.cc:75: #if defined(ANDROID) || V8_TARGET_ARCH_MIPS
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?

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode93
src/heap.cc:93: reserved_semispace_size_(DEFAULT_SEMISPACE_MAX_SIZE),
Here we could set the values to proper default values, like our default values
for non-mobile devices now.

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode177
src/heap.cc:177: #endif
Can you bring back this newline?

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode6688
src/heap.cc:6688: max_executable_size_ = DEFAULT_EXECUTABLE_MAX_SIZE / 2;
The embedder would just set the right values, no memory_constrained guard
needed.

https://codereview.chromium.org/24269003/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/24269003/diff/1/src/heap.h#newcode2342
src/heap.h:2342: bool limits_changed_from_defaults_;
Why do we need that? If the embedder decides to overwrite multiple times that
should be ok? Or is there a problem?

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

https://codereview.chromium.org/24269003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode75
src/heap.cc:75: #if defined(ANDROID) || V8_TARGET_ARCH_MIPS
On 2013/09/19 14:50:57, Hannes Payer wrote:
> 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?

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.

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode93
src/heap.cc:93: reserved_semispace_size_(DEFAULT_SEMISPACE_MAX_SIZE),
On 2013/09/19 14:50:57, Hannes Payer wrote:
> Here we could set the values to proper default values, like our default values
> for non-mobile devices now.

I'm not sure what you mean here?

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode6688
src/heap.cc:6688: max_executable_size_ = DEFAULT_EXECUTABLE_MAX_SIZE / 2;
On 2013/09/19 14:50:57, Hannes Payer wrote:
> The embedder would just set the right values, no memory_constrained guard
> needed.

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).

https://codereview.chromium.org/24269003/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/24269003/diff/1/src/heap.h#newcode2342
src/heap.h:2342: bool limits_changed_from_defaults_;
On 2013/09/19 14:50:57, Hannes Payer wrote:
> Why do we need that? If the embedder decides to overwrite multiple times that
> should be ok? Or is there a problem?

This is not to prevent the embedder overwriting things multiple times (that's
fine, and still works after this CL), it is to prevent the is_memory_constrained
flag resetting the default values if the embedder has already changed the values
from their defaults (i.e., if the embedder specifically sets limits, treat the
preferentially to any changes which would be incurred by setting the
is_memory_constrained flag).

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

https://codereview.chromium.org/24269003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode75
src/heap.cc:75: #if defined(ANDROID) || V8_TARGET_ARCH_MIPS
On 2013/09/19 15:01:34, rmcilroy wrote:
> On 2013/09/19 14:50:57, Hannes Payer wrote:
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode93
src/heap.cc:93: reserved_semispace_size_(DEFAULT_SEMISPACE_MAX_SIZE),
On 2013/09/19 15:01:34, rmcilroy wrote:
> On 2013/09/19 14:50:57, Hannes Payer wrote:
> > Here we could set the values to proper default values, like our default
values
> > for non-mobile devices now.
> 
> I'm not sure what you mean here?

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),

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode6688
src/heap.cc:6688: max_executable_size_ = DEFAULT_EXECUTABLE_MAX_SIZE / 2;
On 2013/09/19 15:01:34, rmcilroy wrote:
> On 2013/09/19 14:50:57, Hannes Payer wrote:
> > The embedder would just set the right values, no memory_constrained guard
> > needed.
> 
> 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). 

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

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

https://codereview.chromium.org/24269003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode75
src/heap.cc:75: #if defined(ANDROID) || V8_TARGET_ARCH_MIPS
On 2013/09/19 16:25:28, Hannes Payer wrote:
> On 2013/09/19 15:01:34, rmcilroy wrote:
> > On 2013/09/19 14:50:57, Hannes Payer wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode93
src/heap.cc:93: reserved_semispace_size_(DEFAULT_SEMISPACE_MAX_SIZE),
On 2013/09/19 16:25:28, Hannes Payer wrote:
> On 2013/09/19 15:01:34, rmcilroy wrote:
> > On 2013/09/19 14:50:57, Hannes Payer wrote:
> > > Here we could set the values to proper default values, like our default
> values
> > > for non-mobile devices now.
> > 
> > I'm not sure what you mean here?
> 
> 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),

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?

https://codereview.chromium.org/24269003/diff/1/src/heap.cc#newcode6688
src/heap.cc:6688: max_executable_size_ = DEFAULT_EXECUTABLE_MAX_SIZE / 2;
On 2013/09/19 16:25:28, Hannes Payer wrote:
> On 2013/09/19 15:01:34, rmcilroy wrote:
> > On 2013/09/19 14:50:57, Hannes Payer wrote:
> > > The embedder would just set the right values, no memory_constrained guard
> > > needed.
> > 
> > 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). 
> 
> But SetResourceConstraints could just set it to the right values. That would
> work, right?

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).

[danno, 2013-09-20 07:12:32]

I don't think adding more #ifdefs to V8 or adding memory_constrained is the
right way to go. The core of V8 is not the best place to consolidate the logic
for the multiple devices and configurations where it needs to run well. Defaults
should be just that, defaults, and I really don't think they should be device or
OS specific (at least at the V8 level) or do probing on the memory system...
then they would no longer be defaults.

I would really like to see this split up. V8 should have device independent
defaults for memory (perhaps except for 32 vs. 64 bits, where they should
double), and I am skeptical about the approach of letting embedder-specific
flags trickle into the core implementation. The embedder should specify
appropriate memory limits, but we can still make this easy by creating a
memory-defaults.cc somewhere in the source tree that is used by d8 and any other
embedder where the collective knowledge of "the right thing to do for a
platform" with the ifdefs and platform/OS-specific probing happens to figure out
what the overrides should be before passing them in.

[rmcilroy, 2013-09-20 11:25:19]

On 2013/09/20 07:12:32, danno wrote:
> I don't think adding more #ifdefs to V8 or adding memory_constrained is the
> right way to go. The core of V8 is not the best place to consolidate the logic
> for the multiple devices and configurations where it needs to run well.
Defaults
> should be just that, defaults, and I really don't think they should be device
or
> OS specific (at least at the V8 level) or do probing on the memory system...
> then they would no longer be defaults.
> 
> I would really like to see this split up. V8 should have device independent
> defaults for memory (perhaps except for 32 vs. 64 bits, where they should
> double), and I am skeptical about the approach of letting embedder-specific
> flags trickle into the core implementation. The embedder should specify
> appropriate memory limits, but we can still make this easy by creating a
> memory-defaults.cc somewhere in the source tree that is used by d8 and any
other
> embedder where the collective knowledge of "the right thing to do for a
> platform" with the ifdefs and platform/OS-specific probing happens to figure
out
> what the overrides should be before passing them in.


To be clear, I was not adding any new #ifdefs here - V8 already does device
specific configuration of the defaults here, this CL just moved the #ifdefs
around to enable runtime information to be taken into account as well.  I
understood that the is_memory_constrained flag was a stopgap measure to isolate
changes made to V8 for low-end devices so we didn't impact performance, but from
the discussion here (and on the Blink CL which plumbed that flag though) it
seems that we should just drop that flag and move to an approach where the V8
embedder sets more fine-grained memory constraint (e.g., total available memory)
on V8, and V8 sizes heap spaces and does other things like code object pre-aging
based on this.  How does this sound to everyone?

I agree that the defaults should not be set in a platform specific way (e.g.,
Android vs. x86, etc.), in part for the very reason I implemented this CL -
there is a massive range in device capability even on just the Android platform.
 However, I do think it is reasonable for V8 to default to reasonable sizes
based on the devices available memory, rather than hard coding
device-independent defaults which only really work well for a single device
(e.g., a Desktop with 2GB of RAM).  WDYT?

If we go down the route of the default being based on values known only at
runtime, then we need some kind of memory probing in V8 (maybe still in
somewhere like memory-defaults.cc) in order to calculate these defaults.  On the
other hand, if we only want to change these values if the embedder specifies
memory constraints, then I think it would be best to have some clear entry point
to initialize V8 where it's explicit to the embedder that they should be setting
these constraints. This is difficult right now due to the default isolate, and I
don't think having this in SetResourceConstraints would work since it is easy to
call this too late (after the heap has been set up), and I don't think many
embedders even call it (Chrome doesn't for the main thread, and only uses it to
set the stack size for worker threads). Any thoughts on this?

IMHO, the defaults should be values which we expect to work well for the current
device V8 is running on, and the embedder should only have to configure V8 if
they know they are doing something unusual (e.g., having many independent
isolates or setting stack limits for worker threads).

I'll try and set up a meeting to discuss this face-to-face, since I would really
like to get a solution which everyone is happy with.

[rmcilroy, 2013-09-23 17:25:46]

As discussed this morning, I've moved this to a API call which sets device
specific heap sizes. WDYT?

I've based the code in platform-X.cc on code in chrome's base::SysInfo class,
but I don't have the platforms available to test these - is there any way I can
test these on trybots for the different platforms?

[Benedikt Meurer, 2013-09-23 20:40:00]

On 2013/09/23 17:25:46, rmcilroy wrote:
> As discussed this morning, I've moved this to a API call which sets device
> specific heap sizes. WDYT?
> 
> I've based the code in platform-X.cc on code in chrome's base::SysInfo class,
> but I don't have the platforms available to test these - is there any way I
can
> test these on trybots for the different platforms?

Please don't spread that over all platform files. We're trying hard to get rid
of them. Add it to platform-posix.cc (with appropriate #if's using V8_OS_*
defines) and platform-win32.cc only. Preferably you would add a SysInfo class to
platform/sysinfo.{h,cc} that handles this (and maybe other things such as
virtual memory limits and page size in the future).

On a more general note: Is it really a good idea to base the decision on the
total physical memory? Because that number does not tell you anything in real
life, when you have several applications resident in memory, or even several
browser tabs open at the same time.

[Hannes Payer (out of office), 2013-09-24 06:39:34]

I think this is a good first step in the right direction. Currently we base
these limits on static ifdefs. Checking for available memory is already an
improvement over the current approach. In an ideal world we would dynamically
check at runtime the system utilization and adjust the memory limits.

Can we adjust the title and the description of the CL?

[Hannes Payer (out of office), 2013-09-24 07:31:00]

https://codereview.chromium.org/24269003/diff/12001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24269003/diff/12001/src/api.cc#newcode636
src/api.cc:636: bool ConfigureResourceConstraintsForCurrentPlatform(
We had another discussion about that function right now... can we move it to a
separate file like v8-defaults.h? Adding it to the api is problematic for many
reasons, e.g., it hard to get rid of stuff added to the api.

https://codereview.chromium.org/24269003/diff/12001/src/api.cc#newcode641
src/api.cc:641: 
Please add a comment which specifies the given configuration values, e.g., young
space has to be a power of 2 and old space has to be a multiple of
Page::kPageSize.

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode72
src/heap.cc:72: #if V8_TARGET_ARCH_X64
Let's get rid of this ifdef as well and use kPointerSize for the default
calculation below.

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode74
src/heap.cc:74: code_range_size_(512*MB),
Can we specify that in ConfigureResourceConstraintsForCurrentPlatform as well?

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode83
src/heap.cc:83: max_executable_size_(256l * LUMP_OF_MEMORY),
indent

[rmcilroy, 2013-09-24 10:50:46]

> Can we adjust the title and the description of the CL?

Done.

> Please don't spread that over all platform files. We're trying hard to get rid
> of them. Add it to platform-posix.cc (with appropriate #if's using V8_OS_*
> defines) and platform-win32.cc only. Preferably you would add a SysInfo class
to
? platform/sysinfo.{h,cc} that handles this (and maybe other things such as
> virtual memory limits and page size in the future).

Done. I'm not particularly keen on the lumping together of code for different
platforms with #ifdefs though - I think it would be cleaner if code for
different platforms was separated out in different files, but then again I also
didn't like the copy/pasted code for platform-linux/openbsd/etc, so fair enough
I will move to this.

[rmcilroy, 2013-09-24 10:50:55]

https://codereview.chromium.org/24269003/diff/12001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24269003/diff/12001/src/api.cc#newcode636
src/api.cc:636: bool ConfigureResourceConstraintsForCurrentPlatform(
On 2013/09/24 07:31:00, Hannes Payer wrote:
> We had another discussion about that function right now... can we move it to a
> separate file like v8-defaults.h? Adding it to the api is problematic for many
> reasons, e.g., it hard to get rid of stuff added to the api.

Done.  Kept the implementation here in api.cc though since that seems to be the
model with other split out includes (e.g., v8-testing.h, etc).

https://codereview.chromium.org/24269003/diff/12001/src/api.cc#newcode641
src/api.cc:641: 
On 2013/09/24 07:31:00, Hannes Payer wrote:
> Please add a comment which specifies the given configuration values, e.g.,
young
> space has to be a power of 2 and old space has to be a multiple of
> Page::kPageSize.

Done.

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode72
src/heap.cc:72: #if V8_TARGET_ARCH_X64
On 2013/09/24 07:31:00, Hannes Payer wrote:
> Let's get rid of this ifdef as well and use kPointerSize for the default
> calculation below.

Done.

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode74
src/heap.cc:74: code_range_size_(512*MB),
On 2013/09/24 07:31:00, Hannes Payer wrote:
> Can we specify that in ConfigureResourceConstraintsForCurrentPlatform as well?

ResourceConstraints doesn't have an option for setting code_range_size.  I'm not
sure if you want to expose that detail to the external API?

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode83
src/heap.cc:83: max_executable_size_(256l * LUMP_OF_MEMORY),
On 2013/09/24 07:31:00, Hannes Payer wrote:
> indent

I'm not sure what you want indented here? The parameters are at the correct
indentation AFAIKS.

[Hannes Payer (out of office), 2013-09-24 11:27:16]

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode74
src/heap.cc:74: code_range_size_(512*MB),
On 2013/09/24 10:50:55, rmcilroy wrote:
> On 2013/09/24 07:31:00, Hannes Payer wrote:
> > Can we specify that in ConfigureResourceConstraintsForCurrentPlatform as
well?
> 
> ResourceConstraints doesn't have an option for setting code_range_size.  I'm
not
> sure if you want to expose that detail to the external API?

I like it how you handle it in your new cl.

https://codereview.chromium.org/24269003/diff/12001/src/heap.cc#newcode83
src/heap.cc:83: max_executable_size_(256l * LUMP_OF_MEMORY),
On 2013/09/24 10:50:55, rmcilroy wrote:
> On 2013/09/24 07:31:00, Hannes Payer wrote:
> > indent
> 
> I'm not sure what you want indented here? The parameters are at the correct
> indentation AFAIKS.

ups, looked strange on codereview, but it is correct.

https://codereview.chromium.org/24269003/diff/23001/include/v8-defaults.h
File include/v8-defaults.h (right):

https://codereview.chromium.org/24269003/diff/23001/include/v8-defaults.h#new...
include/v8-defaults.h:47: * Convience function which performs
SetResourceConstraints with the settings
Convenience?

https://codereview.chromium.org/24269003/diff/23001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24269003/diff/23001/src/api.cc#newcode636
src/api.cc:636: bool ConfigureResourceConstraintsForCurrentPlatform(
We could also move the implementation in a separate file. Danno, what do you
think?

[danno, 2013-09-24 11:52:24]

The general approach seems OK, although I still wish that this could be better
separated from the V8 core (like in another library, which I think would make
sense), but if that's prohibitively expensive, then this approach will do.

https://codereview.chromium.org/24269003/diff/23001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24269003/diff/23001/src/api.cc#newcode636
src/api.cc:636: bool ConfigureResourceConstraintsForCurrentPlatform(
On 2013/09/24 11:27:17, Hannes Payer wrote:
> We could also move the implementation in a separate file. Danno, what do you
> think?

Yes, please put this in another file to make it clear that it's not part of the
core.

https://codereview.chromium.org/24269003/diff/23001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/23001/src/heap.cc#newcode73
src/heap.cc:73: reserved_semispace_size_((kIs64BitArch ? 16 : 8) * MB),
I am not a big fan of the ? here and elsewhere.

How about just 

8 * (kPointerSize / 4) * MB

[rmcilroy, 2013-09-24 13:00:07]

> Yes, please put this in another file to make it clear that > it's not part of
the core.

Done.

https://codereview.chromium.org/24269003/diff/23001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/24269003/diff/23001/src/heap.cc#newcode73
src/heap.cc:73: reserved_semispace_size_((kIs64BitArch ? 16 : 8) * MB),
On 2013/09/24 11:52:24, danno wrote:
> I am not a big fan of the ? here and elsewhere.
> 
> How about just 
> 
> 8 * (kPointerSize / 4) * MB 

Personally I find this more confusing / error prone, but OK - done.

[Hannes Payer (out of office), 2013-09-24 13:43:43]

LGTM from my side, just one nit.

https://codereview.chromium.org/24269003/diff/18001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/24269003/diff/18001/src/globals.h#newcode251
src/globals.h:251: const bool kIs64BitArch = true;
I think these two constants are not used anymore.

[Benedikt Meurer, 2013-09-24 13:47:14]

https://codereview.chromium.org/24269003/diff/18001/src/platform-posix.cc
File src/platform-posix.cc (right):

https://codereview.chromium.org/24269003/diff/18001/src/platform-posix.cc#new...
src/platform-posix.cc:105: uintptr_t OS::TotalPhysicalMemory() {
Shouldn't this return an uint64_t? Otherwise this is going to break in 32-bit
mode applications on a 64-bit machine with more than 4 GiB of RAM, right? Did
you actually test this?

https://codereview.chromium.org/24269003/diff/18001/src/platform-posix.cc#new...
src/platform-posix.cc:116: return static_cast<uintptr_t> size;
This static_cast is missing parenthesis.

[rmcilroy, 2013-09-24 14:39:23]

https://codereview.chromium.org/24269003/diff/18001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/24269003/diff/18001/src/globals.h#newcode251
src/globals.h:251: const bool kIs64BitArch = true;
On 2013/09/24 13:43:44, Hannes Payer wrote:
> I think these two constants are not used anymore.

It's used for the code_range_size_ in heap.cc.  I can change that to use
(kPointerSize == 8) if you prefer, but I think kIs64BitArch is clearer.  WDYT?

https://codereview.chromium.org/24269003/diff/18001/src/platform-posix.cc
File src/platform-posix.cc (right):

https://codereview.chromium.org/24269003/diff/18001/src/platform-posix.cc#new...
src/platform-posix.cc:105: uintptr_t OS::TotalPhysicalMemory() {
On 2013/09/24 13:47:15, Benedikt Meurer wrote:
> Shouldn't this return an uint64_t? Otherwise this is going to break in 32-bit
> mode applications on a 64-bit machine with more than 4 GiB of RAM, right? Did
> you actually test this?

You're right.  For some reason I thought a uintptr_t would be a uint64_t on
64bit machines.  I did test ia32, but the truncation ended up leaving >2GB so I
didn't notice it was truncated.

Done.

https://codereview.chromium.org/24269003/diff/18001/src/platform-posix.cc#new...
src/platform-posix.cc:116: return static_cast<uintptr_t> size;
On 2013/09/24 13:47:15, Benedikt Meurer wrote:
> This static_cast is missing parenthesis.

Done.

[Hannes Payer (out of office), 2013-09-24 15:32:15]

https://codereview.chromium.org/24269003/diff/18001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/24269003/diff/18001/src/globals.h#newcode251
src/globals.h:251: const bool kIs64BitArch = true;
On 2013/09/24 14:39:23, rmcilroy wrote:
> On 2013/09/24 13:43:44, Hannes Payer wrote:
> > I think these two constants are not used anymore.
> 
> It's used for the code_range_size_ in heap.cc.  I can change that to use
> (kPointerSize == 8) if you prefer, but I think kIs64BitArch is clearer.  WDYT?

Ah, yes. Hmm, let's keep the kIs64BitArch constant. That's cleaner.

[rmcilroy, 2013-09-25 12:10:44]

On 2013/09/24 15:32:15, Hannes Payer wrote:
> https://codereview.chromium.org/24269003/diff/18001/src/globals.h
> File src/globals.h (right):
> 
> https://codereview.chromium.org/24269003/diff/18001/src/globals.h#newcode251
> src/globals.h:251: const bool kIs64BitArch = true;
> On 2013/09/24 14:39:23, rmcilroy wrote:
> > On 2013/09/24 13:43:44, Hannes Payer wrote:
> > > I think these two constants are not used anymore.
> > 
> > It's used for the code_range_size_ in heap.cc.  I can change that to use
> > (kPointerSize == 8) if you prefer, but I think kIs64BitArch is clearer. 
WDYT?
> 
> Ah, yes. Hmm, let's keep the kIs64BitArch constant. That's cleaner.

Cool, thanks.

Benedikt: any further comments for the platform code?

[rmcilroy, 2013-09-25 13:02:54]

On 2013/09/25 12:10:44, rmcilroy wrote:
> On 2013/09/24 15:32:15, Hannes Payer wrote:
> > https://codereview.chromium.org/24269003/diff/18001/src/globals.h
> > File src/globals.h (right):
> > 
> > https://codereview.chromium.org/24269003/diff/18001/src/globals.h#newcode251
> > src/globals.h:251: const bool kIs64BitArch = true;
> > On 2013/09/24 14:39:23, rmcilroy wrote:
> > > On 2013/09/24 13:43:44, Hannes Payer wrote:
> > > > I think these two constants are not used anymore.
> > > 
> > > It's used for the code_range_size_ in heap.cc.  I can change that to use
> > > (kPointerSize == 8) if you prefer, but I think kIs64BitArch is clearer. 
> WDYT?
> > 
> > Ah, yes. Hmm, let's keep the kIs64BitArch constant. That's cleaner.
> 
> Cool, thanks.
> 
> Benedikt: any further comments for the platform code?

Danno: could you give this a final LGTM if you're happy with it since Benedikt
is on vacation.

[danno, 2013-09-25 13:07:00]

lgtm

[Hannes Payer (out of office), 2013-09-26 13:31:47]

Committed patchset #6 manually as r16964 (presubmit successful).