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).
https://codereview.chromium.org/24269003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
