Create a new class v8::internal::Isolate in isolate.h/isolate.cc....

Create a new class v8::internal::Isolate in isolate.h/isolate.cc.
Add isolate.cc to the build system.
Move code from v8::internal::V8::Initialize to Isolate::Initialize.

V8::Initialize now calls Isolate::Initialize to do almost
all of its work. From here, statics from the rest of the V8 code will
gradually move into an Isolate object (created on the first call to
Isolate::current() and initialized in V8::initialize). Once the statics have
been moved, the API will be extended to allow for the creation and use of
multiple isolates. Backward compatibility can be maintained using a single
isolate (similar to how this changelist operates).


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

[Vitaly Repeshko, 2010-05-28 14:59:55]

http://codereview.chromium.org/2310003/diff/1/4
File src/isolate.h (right):

http://codereview.chromium.org/2310003/diff/1/4#newcode40
src/isolate.h:40: static Isolate* current();
I think we should only allow lazy initialization in the API layer. Internally we
should be able to assume the current isolate already exists. One of the reasons
is that getting the current isolate is a performance critical thing.

Comment on naming: Isolate::current() makes the reader think it's so fast that
it doesn't really make sense to cache it in a local variable. While we'd like
this to be the case, it still will be slower than that. So Isolate::GetCurrent()
is a better name.

http://codereview.chromium.org/2310003/diff/1/4#newcode51
src/isolate.h:51: bool InitializeFromInternalV8Temp(Deserializer* des);
We'll have a lot of temporary things in this branch. I don't think they deserve
a separate naming convention.

http://codereview.chromium.org/2310003/diff/1/5
File src/v8.cc (right):

http://codereview.chromium.org/2310003/diff/1/5#newcode56
src/v8.cc:56: return Isolate::current()->InitializeFromInternalV8Temp(des);
What about initialization that shouldn't be done for each isolate? E.g.
OS::Setup(), CPU features probing?

We should split initialization into global and per-isolate functions.

[iposva, 2010-05-31 03:48:58]

http://codereview.chromium.org/2310003/diff/1/4
File src/isolate.h (right):

http://codereview.chromium.org/2310003/diff/1/4#newcode40
src/isolate.h:40: static Isolate* current();
On 2010/05/28 14:59:55, Vitaly wrote:
> I think we should only allow lazy initialization in the API layer. Internally
we
> should be able to assume the current isolate already exists. One of the
reasons
> is that getting the current isolate is a performance critical thing.

I don't think the isolate should ever be lazily initialized.

> Comment on naming: Isolate::current() makes the reader think it's so fast that
> it doesn't really make sense to cache it in a local variable. While we'd like
> this to be the case, it still will be slower than that. So
Isolate::GetCurrent()
> is a better name.

What Vitaly is referring to is
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Accessing the current isolate is not a trivial accessor, but I would suggest
that you call the method Current() to match the style guide. The "Get" part is
unnecessary.

http://codereview.chromium.org/2310003/diff/1/5
File src/v8.cc (right):

http://codereview.chromium.org/2310003/diff/1/5#newcode56
src/v8.cc:56: return Isolate::current()->InitializeFromInternalV8Temp(des);
On 2010/05/28 14:59:55, Vitaly wrote:
> What about initialization that shouldn't be done for each isolate? E.g.
> OS::Setup(), CPU features probing?
> 
> We should split initialization into global and per-isolate functions.

I would suggest to add a

static Isolate* Init(Deserializer* des)

to the Isolate class. This would solve the lazy initialization issue that was
raised in Isolate::Current() and gets rid of the InitializeFromInternalV8Temp()
call. In addition the "friend class V8" in isolate.h is obsolete as well.

As far as once-per-process initialization goes I suggest that the different
sub-systems add a static InitOnce in these classes and call them from a central
place. For example I expect that you will need an Isolate::InitOnce() method
down the road.

[Mads Ager (chromium), 2010-05-31 10:48:41]

http://codereview.chromium.org/2310003/diff/8001/9002
File src/isolate.cc (right):

http://codereview.chromium.org/2310003/diff/8001/9002#newcode45
src/isolate.cc:45: static Isolate* gTempGlobalIsolate = NULL;
Drive-by: please use lower-case and underscores for variable names:
global_isolate. The Google style guide allows g_ prefix but I don't think we
have used that anywhere so far.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Naming

[Vitaly Repeshko, 2010-05-31 12:06:11]

http://codereview.chromium.org/2310003/diff/14001/10003
File src/isolate.cc (right):

http://codereview.chromium.org/2310003/diff/14001/10003#newcode56
src/isolate.cc:56: Isolate* newIsolate = new Isolate();
No camel case please. Use this_style for locals.

http://codereview.chromium.org/2310003/diff/14001/10004
File src/isolate.h (right):

http://codereview.chromium.org/2310003/diff/14001/10004#newcode39
src/isolate.h:39: static Isolate* Current();
Make this an inline function. (Declare the static field here and define it in
the .cc file.)

http://codereview.chromium.org/2310003/diff/14001/10004#newcode43
src/isolate.h:43: static Isolate* Init(Deserializer* des);
"Create"? "New"?

http://codereview.chromium.org/2310003/diff/14001/10004#newcode49
src/isolate.h:49: void TearDown();
Should this just be ~Isolate for now? I don't think we can use an isolate after
it has been torn down.

http://codereview.chromium.org/2310003/diff/14001/10004#newcode54
src/isolate.h:54: bool InitInternal(Deserializer* des);
"Init"?

[zarko, 2010-05-31 12:26:51]

Got these; updated the CL.

On 2010/05/31 12:06:11, Vitaly wrote:
> http://codereview.chromium.org/2310003/diff/14001/10003
> File src/isolate.cc (right):
> 
> http://codereview.chromium.org/2310003/diff/14001/10003#newcode56
> src/isolate.cc:56: Isolate* newIsolate = new Isolate();
> No camel case please. Use this_style for locals.
> 
> http://codereview.chromium.org/2310003/diff/14001/10004
> File src/isolate.h (right):
> 
> http://codereview.chromium.org/2310003/diff/14001/10004#newcode39
> src/isolate.h:39: static Isolate* Current();
> Make this an inline function. (Declare the static field here and define it in
> the .cc file.)
> 
> http://codereview.chromium.org/2310003/diff/14001/10004#newcode43
> src/isolate.h:43: static Isolate* Init(Deserializer* des);
> "Create"? "New"?
> 
> http://codereview.chromium.org/2310003/diff/14001/10004#newcode49
> src/isolate.h:49: void TearDown();
> Should this just be ~Isolate for now? I don't think we can use an isolate
after
> it has been torn down.
> 
> http://codereview.chromium.org/2310003/diff/14001/10004#newcode54
> src/isolate.h:54: bool InitInternal(Deserializer* des);
> "Init"?

[Vitaly Repeshko, 2010-05-31 12:32:49]

LGTM.

Moving TearDown-related stuff there seems like a logical next step.


Thanks,
Vitaly

http://codereview.chromium.org/2310003/diff/19001/18004
File src/isolate.h (right):

http://codereview.chromium.org/2310003/diff/19001/18004#newcode58
src/isolate.h:58: bool CreateInternal(Deserializer* des);
One last nitpick. I'd still call this "Init" because it initializes an already
created object.

[zarko, 2010-05-31 12:41:21]

Fixed.

On 2010/05/31 12:32:49, Vitaly wrote:
> LGTM.
> 
> Moving TearDown-related stuff there seems like a logical next step.
> 
> 
> Thanks,
> Vitaly
> 
> http://codereview.chromium.org/2310003/diff/19001/18004
> File src/isolate.h (right):
> 
> http://codereview.chromium.org/2310003/diff/19001/18004#newcode58
> src/isolate.h:58: bool CreateInternal(Deserializer* des);
> One last nitpick. I'd still call this "Init" because it initializes an already
> created object.