Introduce a v8::Platform class that bundles embedder callbacks

Introduce a v8::Platform class that bundles embedder callbacks

Also provide a default implementation to use in cctests.

For now, there are just two thread releated callbacks. In future CLs, I will
move callbacks registered e.g. via V8::SetFooCallback over.

BUG=v8:3015
R=svenpanne@chromium.org, danno@chromium.org
LOG=no

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

[Benedikt Meurer, 2013-11-18 15:34:43]

Thanks for the patch Jochen. First round of comments.

https://codereview.chromium.org/75283002/diff/1/build/features.gypi
File build/features.gypi (right):

https://codereview.chromium.org/75283002/diff/1/build/features.gypi#newcode63
build/features.gypi:63: 'v8_use_default_platform%': 1,
Do we really need this option here? Couldn't we just always include our default
implementation, and in V8::Initialize just check whether g_platform is NULL, and
if so, automagically use the default implementation?

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

https://codereview.chromium.org/75283002/diff/1/include/v8-platform.h#newcode51
include/v8-platform.h:51: class Platform {
Add private copy constructor and assign operator, using V8_DELETE.

https://codereview.chromium.org/75283002/diff/1/include/v8-platform.h#newcode64
include/v8-platform.h:64: virtual void callOnBackgroundThread(Task* task, bool
task_is_slow) = 0;
Nit: First letter should be upper-case.

https://codereview.chromium.org/75283002/diff/1/include/v8-platform.h#newcode71
include/v8-platform.h:71: virtual void callOnForegroundThread(Isolate* isolate,
Task* task) = 0;
Nit: First letter should be upper-case.

https://codereview.chromium.org/75283002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/75283002/diff/1/src/api.cc#newcode5087
src/api.cc:5087: g_platform = new i::DefaultPlatform;
I think platform initialization should be first in V8::Initialize(). If we
really want to be able to move all the platform stuff to the Platform class at
some point, then we will need an initialized Platform to use even basic stuff
such as TLS. So, please do:

if (!g_platform) g_platform = new i::DefaultPlatform;

at the beginning.

[jochen (gone - plz use gerrit), 2013-11-18 16:27:56]

https://codereview.chromium.org/75283002/diff/1/build/features.gypi
File build/features.gypi (right):

https://codereview.chromium.org/75283002/diff/1/build/features.gypi#newcode63
build/features.gypi:63: 'v8_use_default_platform%': 1,
On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> Do we really need this option here? Couldn't we just always include our
default
> implementation, and in V8::Initialize just check whether g_platform is NULL,
and
> if so, automagically use the default implementation?

The default implementation will have to provide e.g. a thread pool, so it's a
non-trivial amount of code which I'd like to avoid including if possible.

Also, V8::Initialize just picking a default implementation is too much magic
IMO. It would lead to unexpected results if we e.g. add a new process in chrome
and forget to set the platform there.

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

https://codereview.chromium.org/75283002/diff/1/include/v8-platform.h#newcode51
include/v8-platform.h:51: class Platform {
On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> Add private copy constructor and assign operator, using V8_DELETE.

It's a pure virtual interface, there's no point in doing so?

https://codereview.chromium.org/75283002/diff/1/include/v8-platform.h#newcode64
include/v8-platform.h:64: virtual void callOnBackgroundThread(Task* task, bool
task_is_slow) = 0;
On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> Nit: First letter should be upper-case.

Done.

https://codereview.chromium.org/75283002/diff/1/include/v8-platform.h#newcode71
include/v8-platform.h:71: virtual void callOnForegroundThread(Isolate* isolate,
Task* task) = 0;
On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> Nit: First letter should be upper-case.

Done.

https://codereview.chromium.org/75283002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/75283002/diff/1/src/api.cc#newcode5087
src/api.cc:5087: g_platform = new i::DefaultPlatform;
On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> I think platform initialization should be first in V8::Initialize(). If we
> really want to be able to move all the platform stuff to the Platform class at
> some point, then we will need an initialized Platform to use even basic stuff
> such as TLS. So, please do:
> 
> if (!g_platform) g_platform = new i::DefaultPlatform;
> 
> at the beginning.

Done (except for the default magic)

[Benedikt Meurer, 2013-11-19 06:57:14]

https://codereview.chromium.org/75283002/diff/1/build/features.gypi
File build/features.gypi (right):

https://codereview.chromium.org/75283002/diff/1/build/features.gypi#newcode63
build/features.gypi:63: 'v8_use_default_platform%': 1,
On 2013/11/18 16:27:56, jochen wrote:
> On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> > Do we really need this option here? Couldn't we just always include our
> default
> > implementation, and in V8::Initialize just check whether g_platform is NULL,
> and
> > if so, automagically use the default implementation?
> 
> The default implementation will have to provide e.g. a thread pool, so it's a
> non-trivial amount of code which I'd like to avoid including if possible.
> 
> Also, V8::Initialize just picking a default implementation is too much magic
> IMO. It would lead to unexpected results if we e.g. add a new process in
chrome
> and forget to set the platform there.

Hm, so the default implementation is not used by Chrome? What worries me is that
we may then run into trouble with reproducing Chrome behavior in d8. We'll have
to regularily sync the default implementation with Chrome, which is certainly
not what we want. Can we do better here?

[jochen (gone - plz use gerrit), 2013-11-19 08:00:46]

On 2013/11/19 06:57:14, Benedikt Meurer wrote:
> https://codereview.chromium.org/75283002/diff/1/build/features.gypi
> File build/features.gypi (right):
> 
> https://codereview.chromium.org/75283002/diff/1/build/features.gypi#newcode63
> build/features.gypi:63: 'v8_use_default_platform%': 1,
> On 2013/11/18 16:27:56, jochen wrote:
> > On 2013/11/18 15:34:44, Benedikt Meurer wrote:
> > > Do we really need this option here? Couldn't we just always include our
> > default
> > > implementation, and in V8::Initialize just check whether g_platform is
NULL,
> > and
> > > if so, automagically use the default implementation?
> > 
> > The default implementation will have to provide e.g. a thread pool, so it's
a
> > non-trivial amount of code which I'd like to avoid including if possible.
> > 
> > Also, V8::Initialize just picking a default implementation is too much magic
> > IMO. It would lead to unexpected results if we e.g. add a new process in
> chrome
> > and forget to set the platform there.
> 
> Hm, so the default implementation is not used by Chrome? What worries me is
that
> we may then run into trouble with reproducing Chrome behavior in d8. We'll
have
> to regularily sync the default implementation with Chrome, which is certainly
> not what we want. Can we do better here?

Yes, it's not used by Chrome.

You can't reproduce chrome behavior in d8 already now, because of all the
callbacks chrome registers, the additional v8::Extensions we inject, and all the
bindings code we add on top of that.

I don't think there's a practical way to share the platform implementation
between v8 and chrome.

[jochen (gone - plz use gerrit), 2013-11-19 13:11:14]

PTAL

[Sven Panne, 2013-11-19 13:21:37]

lgtm

[jochen (gone - plz use gerrit), 2013-11-19 13:44:47]

Committed patchset #3 manually as r17877 (presubmit successful).