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

include/v8-defaults.h

 // 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 17 matching lines @@
 #ifndef V8_V8_DEFAULTS_H_
 #define V8_V8_DEFAULTS_H_
 
 #include "v8.h"
 
 /**
  * Default configuration support for the V8 JavaScript engine.
  */
 namespace v8 {
 
+
+/**
+ * Initializes the v8-defaults subsystem.  Should be called before
+ * ConfigureResourceConstraintsForCurrentPlatform or
+ * SetDefaultResourceConstraintsForCurrentPlatform.
+ */
+void V8_EXPORT InitializeDefaultsForCurrentPlatform(
+    uint64_t total_physical_memory);

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

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

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

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

[danno, 2013/10/25 07:26:15]

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.

+
+
 /**
  * Configures the constraints with reasonable default values based on the
  * capabilities of the current device the VM is running on.
  */
 bool V8_EXPORT ConfigureResourceConstraintsForCurrentPlatform(
     ResourceConstraints* constraints);
 
 
 /**
  * Convience function which performs SetResourceConstraints with the settings
  * returned by ConfigureResourceConstraintsForCurrentPlatform.
  */
 bool V8_EXPORT SetDefaultResourceConstraintsForCurrentPlatform();
 
 }  // namespace v8
 
 #endif  // V8_V8_DEFAULTS_H_