Extension state made per-siolate in genesis

Extension state made per-siolate in genesis


BUG=http://code.google.com/p/v8/issues/detail?id=1821

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

[Dmitry Lomov (no reviews), 2011-11-12 00:14:43]

Fix for http://code.google.com/p/v8/issues/detail?id=1821

[danno, 2011-11-15 09:46:47]

LGTM with nit, although I'd like Vitaly to take a quick look before you land
this.

http://codereview.chromium.org/8536042/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode307
src/api.cc:307: return isolate;
nit: indent

[Vitaly Repeshko, 2011-11-15 18:14:17]

http://codereview.chromium.org/8536042/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode505
src/api.cc:505: 
nit: Remove one blank line.

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode509
src/api.cc:509: reinterpret_cast<uint32_t>(extension));
I'm not sure it'll work with 64 bit pointers. I think you need something like
static_cast<uint32_t>(reinterpret_cast<uintptr_t>(extension)).

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode517
src/api.cc:517: static Allocator default_allocator;
Allocate the allocator with "new" to avoid issues with static destructors.

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode529
src/api.cc:529: entry = Lookup(extension, Hash(extension), true);
Can't you simply return UNVISITED?

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode534
src/api.cc:534: reinterpret_cast<int>(entry->value));
Again, I think it has to be something like
static_cast<ExtensionTraversalState>(reintepret_cast<intptr_t>(entry->value)).

http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode542
src/api.cc:542: }
nit:  // namespace internal

http://codereview.chromium.org/8536042/diff/1/src/api.h
File src/api.h (right):

http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode164
src/api.h:164: class ExtensionStates : private HashMap {
Use delegation instead of implementation inheritance.

http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode170
src/api.h:170: }
nit:  // namespace internal

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

http://codereview.chromium.org/8536042/diff/1/src/isolate.h#newcode814
src/isolate.h:814: ExtensionStates* extension_states() { return
extension_states_; }
It's really unfortunate that this is a part of the isolate state. Please explore
a slightly different approach. Make the InstallExtension family of functions
instance (instead of static) methods of Genesis and move all the extension state
tracking code there. To support the assert in serialize.cc a simple bool flag in
Bootstrapper (e.g. "have_installed_extensions") should be enough.

http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc
File test/cctest/test-lockers.cc (right):

http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#new...
test/cctest/test-lockers.cc:662: context.Dispose();
Dispose the isolate too?

http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#new...
test/cctest/test-lockers.cc:669: TEST(ExtensionsRegistration) {
Please describe what this tests and add a link to the bug.

[Dmitry Lomov (no reviews), 2011-11-15 18:34:43]

Some replies to the comments.
I'll address the ones I didn't reply to.
On 2011/11/15 18:14:17, Vitaly Repeshko wrote:
> http://codereview.chromium.org/8536042/diff/1/src/api.cc
> File src/api.cc (right):
> 
> http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode505
> src/api.cc:505: 
> nit: Remove one blank line.
> 
> http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode509
> src/api.cc:509: reinterpret_cast<uint32_t>(extension));
> I'm not sure it'll work with 64 bit pointers. I think you need something like
> static_cast<uint32_t>(reinterpret_cast<uintptr_t>(extension)).

I just need a hash value, and it does not have to be a perfect hash, so it's all
good.

> 
> http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode517
> src/api.cc:517: static Allocator default_allocator;
> Allocate the allocator with "new" to avoid issues with static destructors.

Allocator is BASE_EMBEDDED, so can't use new.

> 
> http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode529
> src/api.cc:529: entry = Lookup(extension, Hash(extension), true);
> Can't you simply return UNVISITED?
> 
> http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode534
> src/api.cc:534: reinterpret_cast<int>(entry->value));
> Again, I think it has to be something like
> static_cast<ExtensionTraversalState>(reintepret_cast<intptr_t>(entry->value)).

cast from void* to int is always ok in C++.

> 
> http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode542
> src/api.cc:542: }
> nit:  // namespace internal
> 
> http://codereview.chromium.org/8536042/diff/1/src/api.h
> File src/api.h (right):
> 
> http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode164
> src/api.h:164: class ExtensionStates : private HashMap {
> Use delegation instead of implementation inheritance.

All other uses of HashMap in V8 use implementation inheritance.

> 
> http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode170
> src/api.h:170: }
> nit:  // namespace internal
> 
> http://codereview.chromium.org/8536042/diff/1/src/isolate.h
> File src/isolate.h (right):
> 
> http://codereview.chromium.org/8536042/diff/1/src/isolate.h#newcode814
> src/isolate.h:814: ExtensionStates* extension_states() { return
> extension_states_; }
> It's really unfortunate that this is a part of the isolate state. Please
explore
> a slightly different approach. Make the InstallExtension family of functions
> instance (instead of static) methods of Genesis and move all the extension
state
> tracking code there. To support the assert in serialize.cc a simple bool flag
in
> Bootstrapper (e.g. "have_installed_extensions") should be enough.

Yes this is a good idea - another approach would be to just have an
ExtensionStates parameter to InstallExtensions, so as not to pollute Genesis.

> 
> http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc
> File test/cctest/test-lockers.cc (right):
> 
>
http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#new...
> test/cctest/test-lockers.cc:662: context.Dispose();
> Dispose the isolate too?
> 
>
http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#new...
> test/cctest/test-lockers.cc:669: TEST(ExtensionsRegistration) {
> Please describe what this tests and add a link to the bug.

[Dmitry Lomov (no reviews), 2011-11-15 18:42:50]

On 2011/11/15 18:34:43, Dmitry Lomov (chromium) wrote:


> > http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode534
> > src/api.cc:534: reinterpret_cast<int>(entry->value));
> > Again, I think it has to be something like
> >
static_cast<ExtensionTraversalState>(reintepret_cast<intptr_t>(entry->value)).
> 
> cast from void* to int is always ok in C++.
Sorry I take this back. This particular code is ok though, because all values
that come into entry values come from reinterpret_cast<void*>(int value), so it
is good. However, I'll change it to intptr_t so as not to raise people's
eyebrows :)

[Dmitry Lomov (no reviews), 2011-11-15 21:59:20]

Thanks Vitaly for a careful review!
Much more contained patch now, ExtensionStates are not exposed from a Genesis at
all.

[Dmitry Lomov (no reviews), 2011-11-15 22:11:51]

One more thing fixed.
ExtensionStates might be allocated from different threads simultaniously,and a
racy static variable is an undefined behavior. Fixed.

[Vitaly Repeshko, 2011-11-15 22:13:02]

LGTM with a few nits and one real comment.

Thanks!


-- Vitaly

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode214
src/bootstrapper.cc:214: enum ExtensionTraversalState {
nit: Insert a blank line.

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode226
src/bootstrapper.cc:226: };
nit: DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1955: return v8::internal::ComputeIntegerHash(
Oh, actually there's already ComputePointerHash in utils.h :)

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1969: : map_(MatchRegisteredExtensions,
ExtensionStatesAllocator(), 8)
... and does the default allocator (if you omit args 2 and 3 here) work?

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1983: Genesis::ExtensionTraversalState state) {
nit: Formatting.

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:2041: bool
Genesis::InstallExtension(v8::RegisteredExtension* current, ExtensionStates*
extension_states) {
nit: Formatting.

http://codereview.chromium.org/8536042/diff/10001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/8536042/diff/10001/src/isolate.h#newcode898
src/isolate.h:898: void extension_installed() {
Something UpperCase here, e.g. NotifyExtensionInstalled().

http://codereview.chromium.org/8536042/diff/10001/src/isolate.h#newcode1165
src/isolate.h:1165: bool has_installed_extensions_;
Is this initialized somewhere?