Avoid calling OS::TotalPhysicalMemory in defaults.cc, since this fails in the chrome renderer sandb…

Avoid calling OS::TotalPhysicalMemory in defaults.cc, since this fails in the chrome renderer sandbox.

Add an InitializeDefaultsForCurrentPlatform, which the embedder must call before
requesting platform-default ResourceConstraints.

BUG=None

[rmcilroy, 2013-10-24 13:53:18]

Benedikt: please review this CL.

[Benedikt Meurer, 2013-10-24 18:09:02]

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

https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h#newcode45
include/v8-defaults.h:45: uint64_t total_physical_memory);
There was probably some misunderstanding on our side. I thought we settled on
the idea of explicitly initializing a ResourceConstraints object as discussed
with danno. We don't want to have any more global state in V8.

Can you just add the explicit total_physical_memory parameter to
ConfigureResourceConstraintsForCurrentPlatform() below, drop the
SetDefaultResourceConstraintsForCurrentPlatform() method and do the following in
d8.cc (and similar in Blink)?

ResourceConstraints constraints;
if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints,
OS::TotalPhysicalMemory())) { Fatal... }
SetResourceConstraints(isolate, &constraints);

https://codereview.chromium.org/40233002/diff/1/src/defaults.cc
File src/defaults.cc (right):

https://codereview.chromium.org/40233002/diff/1/src/defaults.cc#newcode48
src/defaults.cc:48: static uint64_t g_total_physical_memory = 0;
Please no more global state. See comment in v8-defaults.h.

[rmcilroy, 2013-10-24 21:06:56]

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

https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h#newcode45
include/v8-defaults.h:45: uint64_t total_physical_memory);
On 2013/10/24 18:09:02, Benedikt Meurer wrote:
> There was probably some misunderstanding on our side. I thought we settled on
> the idea of explicitly initializing a ResourceConstraints object as discussed
> with danno. We don't want to have any more global state in V8.
> 
> Can you just add the explicit total_physical_memory parameter to
> ConfigureResourceConstraintsForCurrentPlatform() below, drop the
> SetDefaultResourceConstraintsForCurrentPlatform() method and do the following
in
> d8.cc (and similar in Blink)?
> 
> ResourceConstraints constraints;
> if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints,
> OS::TotalPhysicalMemory())) { Fatal... }
> SetResourceConstraints(isolate, &constraints);

No, this is what I discussed with Daniel over VC and suggested in my email.
Unfortunately we can't pass the total physical memory as a parameter in
ConfigureResourceConstraintsForPlatform() due to the reasons I outlined in the
email thread, namely that Blink can't get the total physical memory due to the
sandbox (same problem as V8), and there is no clean way to plumb through a
resource constraint object from Chrome to the isolate creation points in Blink
(especially for workers where the thread initialisation doesn't enter Chrome
code).

[Benedikt Meurer, 2013-10-25 05:47:19]

On 2013/10/24 21:06:56, rmcilroy wrote:
> https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h
> File include/v8-defaults.h (right):
> 
>
https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h#newcode45
> include/v8-defaults.h:45: uint64_t total_physical_memory);
> On 2013/10/24 18:09:02, Benedikt Meurer wrote:
> > There was probably some misunderstanding on our side. I thought we settled
on
> > the idea of explicitly initializing a ResourceConstraints object as
discussed
> > with danno. We don't want to have any more global state in V8.
> > 
> > Can you just add the explicit total_physical_memory parameter to
> > ConfigureResourceConstraintsForCurrentPlatform() below, drop the
> > SetDefaultResourceConstraintsForCurrentPlatform() method and do the
following
> in
> > d8.cc (and similar in Blink)?
> > 
> > ResourceConstraints constraints;
> > if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints,
> > OS::TotalPhysicalMemory())) { Fatal... }
> > SetResourceConstraints(isolate, &constraints);
> 
> No, this is what I discussed with Daniel over VC and suggested in my email.
> Unfortunately we can't pass the total physical memory as a parameter in
> ConfigureResourceConstraintsForPlatform() due to the reasons I outlined in the
> email thread, namely that Blink can't get the total physical memory due to the
> sandbox (same problem as V8), and there is no clean way to plumb through a
> resource constraint object from Chrome to the isolate creation points in Blink
> (especially for workers where the thread initialisation doesn't enter Chrome
> code).

You don't need to pass the resource constraints. Just pass the total physical
memory (i.e. in some global variable in Blink or Chrome).

[rmcilroy, 2013-10-25 06:05:06]

On 2013/10/25 05:47:19, Benedikt Meurer wrote:
> On 2013/10/24 21:06:56, rmcilroy wrote:
> > https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h
> > File include/v8-defaults.h (right):
> > 
> >
>
https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h#newcode45
> > include/v8-defaults.h:45: uint64_t total_physical_memory);
> > On 2013/10/24 18:09:02, Benedikt Meurer wrote:
> > > There was probably some misunderstanding on our side. I thought we settled
> on
> > > the idea of explicitly initializing a ResourceConstraints object as
> discussed
> > > with danno. We don't want to have any more global state in V8.
> > > 
> > > Can you just add the explicit total_physical_memory parameter to
> > > ConfigureResourceConstraintsForCurrentPlatform() below, drop the
> > > SetDefaultResourceConstraintsForCurrentPlatform() method and do the
> following
> > in
> > > d8.cc (and similar in Blink)?
> > > 
> > > ResourceConstraints constraints;
> > > if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints,
> > > OS::TotalPhysicalMemory())) { Fatal... }
> > > SetResourceConstraints(isolate, &constraints);
> > 
> > No, this is what I discussed with Daniel over VC and suggested in my email.
> > Unfortunately we can't pass the total physical memory as a parameter in
> > ConfigureResourceConstraintsForPlatform() due to the reasons I outlined in
the
> > email thread, namely that Blink can't get the total physical memory due to
the
> > sandbox (same problem as V8), and there is no clean way to plumb through a
> > resource constraint object from Chrome to the isolate creation points in
Blink
> > (especially for workers where the thread initialisation doesn't enter Chrome
> > code).
> 
> You don't need to pass the resource constraints. Just pass the total physical
> memory (i.e. in some global variable in Blink or Chrome).

So to avoid creating a global variable in V8 I should create a global variable
in Blink? I don't see how that is any better.

[Benedikt Meurer, 2013-10-25 06:12:48]

On 2013/10/25 06:05:06, rmcilroy wrote:
> On 2013/10/25 05:47:19, Benedikt Meurer wrote:
> > On 2013/10/24 21:06:56, rmcilroy wrote:
> > > https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h
> > > File include/v8-defaults.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h#newcode45
> > > include/v8-defaults.h:45: uint64_t total_physical_memory);
> > > On 2013/10/24 18:09:02, Benedikt Meurer wrote:
> > > > There was probably some misunderstanding on our side. I thought we
settled
> > on
> > > > the idea of explicitly initializing a ResourceConstraints object as
> > discussed
> > > > with danno. We don't want to have any more global state in V8.
> > > > 
> > > > Can you just add the explicit total_physical_memory parameter to
> > > > ConfigureResourceConstraintsForCurrentPlatform() below, drop the
> > > > SetDefaultResourceConstraintsForCurrentPlatform() method and do the
> > following
> > > in
> > > > d8.cc (and similar in Blink)?
> > > > 
> > > > ResourceConstraints constraints;
> > > > if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints,
> > > > OS::TotalPhysicalMemory())) { Fatal... }
> > > > SetResourceConstraints(isolate, &constraints);
> > > 
> > > No, this is what I discussed with Daniel over VC and suggested in my
email.
> > > Unfortunately we can't pass the total physical memory as a parameter in
> > > ConfigureResourceConstraintsForPlatform() due to the reasons I outlined in
> the
> > > email thread, namely that Blink can't get the total physical memory due to
> the
> > > sandbox (same problem as V8), and there is no clean way to plumb through a
> > > resource constraint object from Chrome to the isolate creation points in
> Blink
> > > (especially for workers where the thread initialisation doesn't enter
Chrome
> > > code).
> > 
> > You don't need to pass the resource constraints. Just pass the total
physical
> > memory (i.e. in some global variable in Blink or Chrome).
> 
> So to avoid creating a global variable in V8 I should create a global variable
> in Blink? I don't see how that is any better.

The point is: If you have to introduce hacks to make it work now, then it's way
better to introduce the hacks on a higher level (esp. since this is issue is
caused by the inability of Blink/Chrome to properly forward the resource
constraint/total physical memory information to the appropriate place), since
it's way easier to get rid of hacks later in a top-down fashion than in a
bottom-up fashion.

[Sven Panne, 2013-10-25 06:27:16]

NOT LGTM. We won't accept extremely ugly hacks in v8 to solve problems which are
clearly on the embedder side (i.e. Blink). I understand that there is some
threading needed in Blink to get information from the
"before-the-sandbox-is-sealed" to the "after-the-sandbox-is-sealed" state, but
this is purely are problem for Blink, and I'm quite sure that this problem has
already solved in Blink somehow already. If not, fixing that in Blink would be
the right approach. Abusing v8 as a kind of global state to cheat and make some
Blink owners feel better is not the way to go. Our external API is already ugly,
let's not make it worse...

https://codereview.chromium.org/40233002/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/40233002/diff/1/src/d8.cc#newcode1664
src/d8.cc:1664: ASSERT(v8::SetDefaultResourceConstraintsForCurrentPlatform());
Huh? A side effect in an assert? This is definitely not what you want...

[danno, 2013-10-25 07:26:14]

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

https://codereview.chromium.org/40233002/diff/1/include/v8-defaults.h#newcode45
include/v8-defaults.h:45: uint64_t total_physical_memory);
On 2013/10/24 21:06:56, rmcilroy wrote:
> On 2013/10/24 18:09:02, Benedikt Meurer wrote:
> > There was probably some misunderstanding on our side. I thought we settled
on
> > the idea of explicitly initializing a ResourceConstraints object as
discussed
> > with danno. We don't want to have any more global state in V8.
> > 
> > Can you just add the explicit total_physical_memory parameter to
> > ConfigureResourceConstraintsForCurrentPlatform() below, drop the
> > SetDefaultResourceConstraintsForCurrentPlatform() method and do the
following
> in
> > d8.cc (and similar in Blink)?
> > 
> > ResourceConstraints constraints;
> > if (!ConfigureResourceConstraintsForCurrentPlatform(&constraints,
> > OS::TotalPhysicalMemory())) { Fatal... }
> > SetResourceConstraints(isolate, &constraints);
> 
> No, this is what I discussed with Daniel over VC and suggested in my email.
> Unfortunately we can't pass the total physical memory as a parameter in
> ConfigureResourceConstraintsForPlatform() due to the reasons I outlined in the
> email thread, namely that Blink can't get the total physical memory due to the
> sandbox (same problem as V8), and there is no clean way to plumb through a
> resource constraint object from Chrome to the isolate creation points in Blink
> (especially for workers where the thread initialisation doesn't enter Chrome
> code).

I apologize, this misunderstanding appears to be my fault. I misinterpreted your
proposal, I thought we were just discussing where
ConfigureResourceConstraintsForPlatform needs to be called and not passing
around the ResourceConstraint struct through Blink in our VC. I thought you were
proposing adding the size to the existing
ConfigureResourceConstraintsForCurrentPlatform call, not creating a new one.
Sorry, may bad.

I do see a fundamental difference between passing around the total memory size
and the ResourceConstraints struct in Chrome/Blink. The former seems like
something that is useful in other contexts, and therefore I'm sure that it's
possible to get the support to thread this through, while the second is
V8-specific. So, I think "right" thing to do is indeed pass the total physical
memory to ConfigureResourceConstraintsForPlatform(), and call that in the
sandbox immediately before SetResourceConstraints.

If you need help threading this through and somebody to help shepherd you CL,
I'd recommend getting in touch with Jochen Eisinger. He's in MUC and has the
clout to help get a change like this through.