Add a ResourceConstraint for the embedder to specify that V8 is running on a memory constrained dev…

Add a ResourceConstraint for the embedder to specify that V8 is running on a memory constrained device.

This enables us to specialize certain operations such that we limit memory
usage on low-memory devices, without reducing performance on devices which
are not memory constrained.

BUG=chromium:280984
R=svenpanne@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=16608

[rmcilroy, 2013-09-03 16:03:15]

Hi Sven, could you please review this change. An example of how this flag will
be used is in https://codereview.chromium.org/23480031/.

[tfarina, 2013-09-03 19:39:17]

https://codereview.chromium.org/23464022/diff/1/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/23464022/diff/1/src/isolate.h#newcode1141
src/isolate.h:1141: bool isMemoryConstrained() {
I believe this should be is_memory_constrained()

https://codereview.chromium.org/23464022/diff/1/src/isolate.h#newcode1144
src/isolate.h:1144: void setIsMemoryConstrained(bool value) {
set_is_memory_constrained

[Sven Panne, 2013-09-04 07:25:45]

Adding Michael, we talked about this yesterday. In general this looks OK, but it
is of course only the threading of a flag. It would be nice to see a CL building
on this one where the new predicate is actually used (plus some hints where it
might be used, too).

The change as it is isolates things a bit, but the new flag is still "a kind of
magic". (Queen pun intended ;-)

https://chromiumcodereview.appspot.com/23464022/diff/1/src/isolate.h
File src/isolate.h (right):

https://chromiumcodereview.appspot.com/23464022/diff/1/src/isolate.h#newcode1141
src/isolate.h:1141: bool isMemoryConstrained() {
On 2013/09/03 19:39:18, tfarina wrote:
> I believe this should be is_memory_constrained()

... and should be const.

[rmcilroy, 2013-09-04 10:22:06]

Thanks Sven and tfarina.  

Sure thing, as I mentioned in the initial message,
https://codereview.chromium.org/23480031/ shows the first intended use of this
flag. I envisage quite a few more, for example, reducing the amount of code
inlining, changing the parameters which decide between expanding the heap or
performing GC to prefer GC on memory constrained devices, changing the amount by
which the heap is expanded (e.g., expand by 256KB rather than 1MB each time to
have more granularity), and tweaking the max_heap default parameters based on
the flag.  I'll have to investigate these first before I can work out whether
they will be worth it though.

https://codereview.chromium.org/23464022/diff/1/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/23464022/diff/1/src/isolate.h#newcode1141
src/isolate.h:1141: bool isMemoryConstrained() {
On 2013/09/04 07:25:45, Sven Panne wrote:
> On 2013/09/03 19:39:18, tfarina wrote:
> > I believe this should be is_memory_constrained()
> 
> ... and should be const.

Done.

https://codereview.chromium.org/23464022/diff/1/src/isolate.h#newcode1144
src/isolate.h:1144: void setIsMemoryConstrained(bool value) {
On 2013/09/03 19:39:18, tfarina wrote:
> set_is_memory_constrained

Done.

[Sven Panne, 2013-09-05 08:49:45]

LGTM if there is no veto from Michael. I think the right way to proceed is to
use the new predicate for a few low-memory-device CLs and perhaps refactor the
external API a bit if possible. I am not 100% happy with the introduction of
magic flags, but currently I don't know a better alternative. Perhaps a comment
in v8.h saying that this is a tentative addition to the API might be
appropriate.

[rmcilroy, 2013-09-05 15:36:39]

On 2013/09/05 08:49:45, Sven Panne wrote:
> LGTM if there is no veto from Michael. I think the right way to proceed is to
> use the new predicate for a few low-memory-device CLs and perhaps refactor the
> external API a bit if possible. I am not 100% happy with the introduction of
> magic flags, but currently I don't know a better alternative. Perhaps a
comment
> in v8.h saying that this is a tentative addition to the API might be
> appropriate.

Thanks Sven.  I've had to slightly change the API to take a pointer to a boolean
so that the setResourceConstraints will not re-set is_memory_constrained if the
ResourceConstraint being passed didn't have a call to set_memory_constrained
(e.g., treat the value as either do-nothing, set to true or set to false).  I
had to do this because this will be called by both Chrome code and Blink code,
so I need to have a "do nothing" option to avoid Chrome setting it to true, and
blink accidentally setting back to the default of false.

PTAL.  If you are happy with it could you please land it for me (I'm not a V8
committer yet).

[Sven Panne, 2013-09-05 17:28:37]

On 2013/09/05 15:36:39, rmcilroy wrote:
> Thanks Sven.  I've had to slightly change the API to take a pointer to a
boolean
> so that the setResourceConstraints will not re-set is_memory_constrained if
the
> ResourceConstraint being passed didn't have a call to set_memory_constrained
> (e.g., treat the value as either do-nothing, set to true or set to false).  I
> had to do this because this will be called by both Chrome code and Blink code,
> so I need to have a "do nothing" option to avoid Chrome setting it to true,
and
> blink accidentally setting back to the default of false.
> 
> PTAL.  If you are happy with it could you please land it for me (I'm not a V8
> committer yet).

NOT LGTM. The plan regarding Isolates and ResourceConstraints is that there will
be exactly one way to create an Isolate (no default Isolate anymore) via some
constructor or factory method, and this constructor/factory will get optional
ResourceConstraints (or something similar). Therefore there should be exactly
*one* place to construct ResourceConstraints, and you should basically view them
as immutable. Put another way: ResourceConstraints are just additional arguments
to the Isolate constructor/factory, i.e. it's an argument object.

Perhaps I misunderstood things, so we should clarify this tomorrow or next week,
but giving out a pointer to parts of that argument object seems fundamentally
wrong.

[rmcilroy, 2013-09-05 18:29:59]

On 2013/09/05 17:28:37, Sven Panne wrote:
> On 2013/09/05 15:36:39, rmcilroy wrote:
> > Thanks Sven.  I've had to slightly change the API to take a pointer to a
> boolean
> > so that the setResourceConstraints will not re-set is_memory_constrained if
> the
> > ResourceConstraint being passed didn't have a call to set_memory_constrained
> > (e.g., treat the value as either do-nothing, set to true or set to false). 
I
> > had to do this because this will be called by both Chrome code and Blink
code,
> > so I need to have a "do nothing" option to avoid Chrome setting it to true,
> and
> > blink accidentally setting back to the default of false.
> > 
> > PTAL.  If you are happy with it could you please land it for me (I'm not a
V8
> > committer yet).
> 
> NOT LGTM. The plan regarding Isolates and ResourceConstraints is that there
will
> be exactly one way to create an Isolate (no default Isolate anymore) via some
> constructor or factory method, and this constructor/factory will get optional
> ResourceConstraints (or something similar). Therefore there should be exactly
> *one* place to construct ResourceConstraints, and you should basically view
them
> as immutable. Put another way: ResourceConstraints are just additional
arguments
> to the Isolate constructor/factory, i.e. it's an argument object.
> 
> Perhaps I misunderstood things, so we should clarify this tomorrow or next
week,
> but giving out a pointer to parts of that argument object seems fundamentally
> wrong.

Sorry, I think we are misunderstanding.  There is still only a single
ResourceConstraint (which is in-effect immutable and is treated as an argument
object).  There is no intention that if the embedder changes the
ResourceConstraint object after they have called setResourceConstraints, that
this would have any effect.  It is only that both Blink and Chrome will create
their own ResourceConstraint object (setting different settings on this
ResourceConstraint), and call setResourceConstraints on them independently.  I
changed the bool to a bool* in order to have a NULL (i.e., unset) as well as
true/false for the constraint, like the other constraints do (e.g., having '0'
as being unset).  I originally used an enum for this instead of making it a
pointer, but thought it was a bit clunky, but can go back to that model if you
don't like this being a pointer?

[Sven Panne, 2013-09-06 07:38:00]

On 2013/09/05 18:29:59, rmcilroy wrote:
> Sorry, I think we are misunderstanding.  There is still only a single
> ResourceConstraint (which is in-effect immutable and is treated as an argument
> object).  There is no intention that if the embedder changes the
> ResourceConstraint object after they have called setResourceConstraints, that
> this would have any effect.  It is only that both Blink and Chrome will create
> their own ResourceConstraint object (setting different settings on this
> ResourceConstraint), and call setResourceConstraints on them independently.  I
> changed the bool to a bool* in order to have a NULL (i.e., unset) as well as
> true/false for the constraint, like the other constraints do (e.g., having '0'
> as being unset).  I originally used an enum for this instead of making it a
> pointer, but thought it was a bit clunky, but can go back to that model if you
> don't like this being a pointer?

Using an enum is one possibility, but I think we could make things much cleaner
when we move our internal Maybe template into the v8.h header and let the
ResourceConstraints getter return a Maybe<foo> instead of foo.

I had a look at the Chrome/Blink code, and I am not sure where one would set the
ResourceConstraints for the main thread (workers are easy, they have an explicit
Isolate::New()). The current API is extremely fragile, because you have to call
SetResourceConstaints very, very early. Adding Dan...

[rmcilroy_google, 2013-09-06 09:18:55]

On 2013/09/06 07:38:00, Sven Panne wrote:
> On 2013/09/05 18:29:59, rmcilroy wrote:
> > Sorry, I think we are misunderstanding.  There is still only a single
> > ResourceConstraint (which is in-effect immutable and is treated as an
argument
> > object).  There is no intention that if the embedder changes the
> > ResourceConstraint object after they have called setResourceConstraints,
that
> > this would have any effect.  It is only that both Blink and Chrome will
create
> > their own ResourceConstraint object (setting different settings on this
> > ResourceConstraint), and call setResourceConstraints on them independently. 
I
> > changed the bool to a bool* in order to have a NULL (i.e., unset) as well as
> > true/false for the constraint, like the other constraints do (e.g., having
'0'
> > as being unset).  I originally used an enum for this instead of making it a
> > pointer, but thought it was a bit clunky, but can go back to that model if
you
> > don't like this being a pointer?
> 
> Using an enum is one possibility, but I think we could make things much
cleaner
> when we move our internal Maybe template into the v8.h header and let the
> ResourceConstraints getter return a Maybe<foo> instead of foo.
> 
> I had a look at the Chrome/Blink code, and I am not sure where one would set
the
> ResourceConstraints for the main thread (workers are easy, they have an
explicit
> Isolate::New()). The current API is extremely fragile, because you have to
call
> SetResourceConstaints very, very early. Adding Dan...

My plan for the main thread was to call it in RenderThreadImpl
(https://codereview.chromium.org/24000003/).  This should be early enough
shouldn't it?  It was actually the workers which I was finding more difficult to
find a place since I need call this from Chromium code (the Blink folks are not
keen to add the plumbing to pass SysUtils::isLowEndDevice through to Blink to
then set this, so I need to call it from Chrome code).  Do you know of anywhere
early in the worker's initialization which is called in the chromium layer (as
far as I can see, the Isolate::New() call is in the Blink side).

[Sven Panne, 2013-09-06 09:35:56]

My gut feeling is that one should thread the isLowEndDevice info through Blink,
so that both the main thread and the workers know what they should create. It is
Blink after all which creates Isolates (or for the main thread: which should
create it in the future), so it should get everything it needs, if other Blink
people like it or not.

[dcarney, 2013-09-06 11:17:37]

On 2013/09/06 09:35:56, Sven Panne wrote:
> My gut feeling is that one should thread the isLowEndDevice info through
Blink,
> so that both the main thread and the workers know what they should create. It
is
> Blink after all which creates Isolates (or for the main thread: which should
> create it in the future), so it should get everything it needs, if other Blink
> people like it or not.

Yeah, I'm with Sven here.  Somehow chrome needs to tell blink to tell v8 what to
do at isolate initialization time.  Otherwise, layering violations will occur
once we have isolate initialization be explicit.

[rmcilroy, 2013-09-09 17:28:21]

Updated to use the Maybe template.  PTAL.

[Sven Panne, 2013-09-10 06:09:56]

LGTM with a nit.

https://codereview.chromium.org/23464022/diff/21001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/23464022/diff/21001/include/v8.h#newcode3816
include/v8.h:3816: Maybe<bool> is_memory_constrained() { return
is_memory_constrained_; }
Missing const.

[rmcilroy, 2013-09-10 10:28:52]

On 2013/09/10 06:09:56, Sven Panne wrote:
> LGTM with a nit.
> 
> https://codereview.chromium.org/23464022/diff/21001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/23464022/diff/21001/include/v8.h#newcode3816
> include/v8.h:3816: Maybe<bool> is_memory_constrained() { return
> is_memory_constrained_; }
> Missing const.

Thanks Sven, updated the CL to address your comments.

Could you please land this CL for me (and the Maybe template moving CL which
this CL depends upon).

[Sven Panne, 2013-09-10 10:57:11]

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