Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566002:
[Isolates] Isolate API: first draft. (Closed)
|
Created:
10 years, 6 months ago by Vitaly Repeshko Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
Description[Isolates] Isolate API: first draft.
Committed: http://code.google.com/p/v8/source/detail?r=5004
Patch Set 1 #Patch Set 2 : Just the API #
Total comments: 15
Patch Set 3 : Review fixes #Patch Set 4 : Rebased on ToT #
Total comments: 10
Patch Set 5 : Review fixes #
Total comments: 7
Messages
Total messages: 16 (0 generated)
http://codereview.chromium.org/2566002/diff/3001/4001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/3001/4001#newcode2448 include/v8.h:2448: Isolate& operator=(const Isolate&); don't we need internal::Isolate* isolate_; as well as class declaration in namespace v8 { namespace internal { class Isolate; } } ?
Initial set of comments as discussed offline. http://codereview.chromium.org/2566002/diff/3001/4001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/3001/4001#newcode2388 include/v8.h:2388: * Isolate represents an isolated instace of V8 engine. V8 isolates V8 engine -> V8 or V8 engine -> the V8 engine http://codereview.chromium.org/2566002/diff/3001/4001#newcode2390 include/v8.h:2390: * be used in other isolates. When V8 initializes a default isolate initializes -> is initialized? http://codereview.chromium.org/2566002/diff/3001/4001#newcode2392 include/v8.h:2392: * additional isolates and run them in parallel in multiple run them -> use them? http://codereview.chromium.org/2566002/diff/3001/4001#newcode2394 include/v8.h:2394: * given time. Locker/Unlocker API can be used to synchronize. Locker/Unlocker API -> The Locker/Unlocker API http://codereview.chromium.org/2566002/diff/3001/4001#newcode2421 include/v8.h:2421: static Isolate* New(); As discussed offline, we should have an explicit dispose method so we do not get into situations where memory allocated inside of V8 has to be deallocated outside of V8. If V8 is used as a shared library by code using a different allocator this could lead to trouble. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2432 include/v8.h:2432: * restored by when exiting. Re-entering an isolate is allowed. by when -> when
Please take another look. Thanks, Vitaly http://codereview.chromium.org/2566002/diff/3001/4001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/3001/4001#newcode2388 include/v8.h:2388: * Isolate represents an isolated instace of V8 engine. V8 isolates On 2010/06/03 14:05:46, Mads Ager wrote: > V8 engine -> V8 or V8 engine -> the V8 engine Done. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2390 include/v8.h:2390: * be used in other isolates. When V8 initializes a default isolate On 2010/06/03 14:05:46, Mads Ager wrote: > initializes -> is initialized? Done. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2392 include/v8.h:2392: * additional isolates and run them in parallel in multiple On 2010/06/03 14:05:46, Mads Ager wrote: > run them -> use them? Done. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2394 include/v8.h:2394: * given time. Locker/Unlocker API can be used to synchronize. On 2010/06/03 14:05:46, Mads Ager wrote: > Locker/Unlocker API -> The Locker/Unlocker API Done. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2421 include/v8.h:2421: static Isolate* New(); On 2010/06/03 14:05:46, Mads Ager wrote: > As discussed offline, we should have an explicit dispose method so we do not get > into situations where memory allocated inside of V8 has to be deallocated > outside of V8. If V8 is used as a shared library by code using a different > allocator this could lead to trouble. Done. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2432 include/v8.h:2432: * restored by when exiting. Re-entering an isolate is allowed. On 2010/06/03 14:05:46, Mads Ager wrote: > by when -> when Done. http://codereview.chromium.org/2566002/diff/3001/4001#newcode2448 include/v8.h:2448: Isolate& operator=(const Isolate&); On 2010/06/03 12:49:40, Maxim.Mossienko wrote: > don't we need internal::Isolate* isolate_; as well as class declaration in > namespace v8 { namespace internal { class Isolate; } } ? Not really. We can reuse "this" pointer.
http://codereview.chromium.org/2566002/diff/3001/4001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/3001/4001#newcode2448 include/v8.h:2448: Isolate& operator=(const Isolate&); I am fine if there will be no list / map for storing v8::Isolate -> v8::internal::Isolate relation On 2010/06/03 15:06:57, Vitaly wrote: > On 2010/06/03 12:49:40, Maxim.Mossienko wrote: > > don't we need internal::Isolate* isolate_; as well as class declaration in > > namespace v8 { namespace internal { class Isolate; } } ? > > Not really. We can reuse "this" pointer. >
http://codereview.chromium.org/2566002/diff/12001/4 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/12001/4#newcode2424 include/v8.h:2424: static Isolate* New(); Do we also need New(ResourceConstraints*) variant?
http://codereview.chromium.org/2566002/diff/12001/4 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/12001/4#newcode2387 include/v8.h:2387: /** What bothers me about this API is the implicit chaining of isolates. Basically you have to ensure that a thread exits the current isolate before starting to use a new isolate, but unfortunately with this API you can't. There is a reference to the first isolate created by the thread at the bottom of the chain. Not sure I have a good solution for this though. For example: ... <code initializing V8> // If the lines below are missing, the isolate initializing V8 // cannot be disposed. V8::Isolate::GetCurrent()->Exit(); // oops, private api ... while (should_continue()) { Request* req = Request::getNext(); V8::Isolate::Scope(req->getIsolate()); ... <handle request in req specific isolate> ... } ... http://codereview.chromium.org/2566002/diff/12001/4#newcode2424 include/v8.h:2424: static Isolate* New(); How do you set per isolate specific ResourceConstraints using this API? I could imagine that different isolates want to allow different heap sizes. How about adding a ResourceConstraints parameter? http://codereview.chromium.org/2566002/diff/12001/4#newcode2433 include/v8.h:2433: * Methods below this point require holding a lock in multi-threaded in multi-threaded -> in a multi-threaded http://codereview.chromium.org/2566002/diff/12001/4#newcode2441 include/v8.h:2441: void Dispose(); Do you assert in debug mode that Dispose is not called on an isolate still chained?
> What bothers me about this API is the implicit chaining of isolates. Basically > you have to ensure that a thread exits the current isolate before starting to > use a new isolate, but unfortunately with this API you can't. There is a > reference to the first isolate created by the thread at the bottom of the chain. > > Not sure I have a good solution for this though. Would it be ok if Dispose was a static method and could be called on the currently entered isolate so it would exit it internally? So far this is the only non-static public method.
http://codereview.chromium.org/2566002/diff/12001/4 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/12001/4#newcode2387 include/v8.h:2387: /** On 2010/06/03 17:02:09, iposva wrote: > What bothers me about this API is the implicit chaining of isolates. Basically > you have to ensure that a thread exits the current isolate before starting to > use a new isolate, but unfortunately with this API you can't. There is a > reference to the first isolate created by the thread at the bottom of the chain. > > Not sure I have a good solution for this though. > > For example: > > ... > <code initializing V8> > // If the lines below are missing, the isolate initializing V8 > // cannot be disposed. > V8::Isolate::GetCurrent()->Exit(); // oops, private api > ... > while (should_continue()) { > Request* req = Request::getNext(); > V8::Isolate::Scope(req->getIsolate()); > ... > <handle request in req specific isolate> > ... > } > ... > We decided to expose Enter/Exit back as they were in the initial patch set. http://codereview.chromium.org/2566002/diff/12001/4#newcode2424 include/v8.h:2424: static Isolate* New(); On 2010/06/03 16:47:50, Dmitry Titov wrote: > Do we also need New(ResourceConstraints*) variant? Yes, I'm going to add that later. I don't want to specify termination conditions yet. http://codereview.chromium.org/2566002/diff/12001/4#newcode2424 include/v8.h:2424: static Isolate* New(); On 2010/06/03 17:02:09, iposva wrote: > How do you set per isolate specific ResourceConstraints using this API? I could > imagine that different isolates want to allow different heap sizes. How about > adding a ResourceConstraints parameter? This is exactly what we're going to do. See my reply to Dmitry's comment. http://codereview.chromium.org/2566002/diff/12001/4#newcode2433 include/v8.h:2433: * Methods below this point require holding a lock in multi-threaded On 2010/06/03 17:02:09, iposva wrote: > in multi-threaded -> in a multi-threaded Done. http://codereview.chromium.org/2566002/diff/12001/4#newcode2441 include/v8.h:2441: void Dispose(); On 2010/06/03 17:02:09, iposva wrote: > Do you assert in debug mode that Dispose is not called on an isolate still > chained? Yes, this is what we plan to do. In general, we're going to add a lot of debug checks to ensure transition to multi-isolate world is smooth (for those who need it).
The patch LGTM :-) Mads, Ivan - is this shape of API ok to go? We practically finished conversion of statics and need an ability to create more then one isolate :-) So we need some API. Can do internal test but it's more fun with API.
The API looks good to me and we can iterate on it once it is in. You should implement the API so we can start using it for testing. On 2010/06/30 22:03:49, Dmitry Titov wrote: > The patch LGTM :-) > > Mads, Ivan - is this shape of API ok to go? > > We practically finished conversion of statics and need an ability to create more > then one isolate :-) So we need some API. Can do internal test but it's more fun > with API.
http://codereview.chromium.org/2566002/diff/17001/18001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/17001/18001#newcode2388 include/v8.h:2388: * Isolate represents an isolated instace of the V8 engine. V8 typo: instace -> instance http://codereview.chromium.org/2566002/diff/17001/18001#newcode2397 include/v8.h:2397: public: I think that if we are to preserve API compatibility then many of V8 static methods should be instance ones in Isolate (e.g. SetFatalErrorHandler, IsDead, SetCounterFunction, etc). This would just reflect the fact of Isolate being instance of V8. http://codereview.chromium.org/2566002/diff/17001/18001#newcode2433 include/v8.h:2433: * Methods below this point require holding a lock in a IMO bare 'lock' word is confusing in the description (given Locker class existence), 'caller managed exclusive thread access' is better description
http://codereview.chromium.org/2566002/diff/17001/18001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/17001/18001#newcode2394 include/v8.h:2394: * given time. The Locker/Unlocker API can be used to synchronize. This is not currently possible in this API draft. Locker / Unlocker constructor need to accept Isolate instance to be able to synchronize. http://codereview.chromium.org/2566002/diff/17001/18001#newcode2461 include/v8.h:2461: Isolate(); Following Locker statics go here: /** * Returns whether v8::Locker is being used by this V8 instance. */ bool IsActive() { return active_; } // Track whether this V8 instance has ever called v8::Locker. This allows the // API code to verify that the lock is always held when V8 is being entered. bool active_;
Maxim, you have some good comments about possible future steps, indeed Lockers are not yet functional with Isolates. We plan to first enable multiple isolates and run them w/o Lockers, on separate threads. This should be possible and gives us a lot of bugs to fix (heh). So we are moving step by step, and if Vitaly lands this change, we can go ahead and implement Isolate API object and start writing tests.
Thanks everyone for comments! This is now landed. -- Vitaly http://codereview.chromium.org/2566002/diff/17001/18001 File include/v8.h (right): http://codereview.chromium.org/2566002/diff/17001/18001#newcode2388 include/v8.h:2388: * Isolate represents an isolated instace of the V8 engine. V8 On 2010/07/01 08:31:50, Maxim.Mossienko wrote: > typo: instace -> instance Fixed. http://codereview.chromium.org/2566002/diff/17001/18001#newcode2433 include/v8.h:2433: * Methods below this point require holding a lock in a On 2010/07/01 08:31:50, Maxim.Mossienko wrote: > IMO bare 'lock' word is confusing in the description (given Locker class > existence), 'caller managed exclusive thread access' is better description Clarified how the embedder is supposed to hold a lock.
What's the status of Isolate? On 2010/07/01 22:00:57, Vitaly wrote: > Thanks everyone for comments! This is now landed. > > > -- Vitaly > > http://codereview.chromium.org/2566002/diff/17001/18001 > File include/v8.h (right): > > http://codereview.chromium.org/2566002/diff/17001/18001#newcode2388 > include/v8.h:2388: * Isolate represents an isolated instace of the V8 engine. > V8 > On 2010/07/01 08:31:50, Maxim.Mossienko wrote: > > typo: instace -> instance > > Fixed. > > http://codereview.chromium.org/2566002/diff/17001/18001#newcode2433 > include/v8.h:2433: * Methods below this point require holding a lock in a > On 2010/07/01 08:31:50, Maxim.Mossienko wrote: > > IMO bare 'lock' word is confusing in the description (given Locker class > > existence), 'caller managed exclusive thread access' is better description > > Clarified how the embedder is supposed to hold a lock. |
