ported --isolate option to d8 and refactored to group together option parsing

ported --isolate option to d8 and refactored to group together option parsing

TEST=tools/test.py -j15 --shell d8 --isolates

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

[Yang, 2011-07-07 16:04:49]

Made some big changes in d8:
- ported the --isolate option from the sample shell. There are some differences
because the sample shell does not use lockers.
- moved the entire parsing of command line options to a stand-alone method
(SetOptions), keeping the parsed result in a passive static object
(Shell::options)

http://codereview.chromium.org/7318002/diff/1/src/d8.h
File src/d8.h (right):

http://codereview.chromium.org/7318002/diff/1/src/d8.h#newcode115
src/d8.h:115: class SourceGroup {
class necessary for the --isolate option

http://codereview.chromium.org/7318002/diff/1/src/d8.h#newcode173
src/d8.h:173: class ShellOptions {
grouped all the options for the shell in here.

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode45
src/d8.cc:45: #include <unistd.h>  // NOLINT
necessary for _exit()

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode633
src/d8.cc:633: void Shell::Initialize() {
test_shell is now a part of Shell::options and therefore static

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode676
src/d8.cc:676: context_mutex_->Lock();
mark critical section because creating a context is not thread-safe

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode724
src/d8.cc:724: v8::Unlocker unlocker(Isolate::GetCurrent());
the default constructor takes the default isolate, which is *not* the same as
the current isolate.

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode824
src/d8.cc:824: // Prepare the context for this thread.
no point in locking earlier than necessary

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode853
src/d8.cc:853: 
reused code from sample/shell.cc

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode936
src/d8.cc:936: Locker lock(isolate);
the default constructor takes the default isolate, which is *not* the same as
the current isolate. therefore we explicitly use the new isolate. otherwise the
locker will actually enter the default isolate, making this whole construct
pointless.

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode971
src/d8.cc:971: bool Shell::SetOptions(int argc, char* argv[]) {
moved all the options parsing into this. the parsed options are then stored in
Shell::options

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode1033
src/d8.cc:1033: printf("-p is not compatible with --isolate\n");
either we have parallelism with -p or with --isolate, but not both! the
difference is that -p works on the same isolate.

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode1077
src/d8.cc:1077: if (options.parallel_files != NULL)
if parallel_files != NULL then there obviously are no --isolate options

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode1096
src/d8.cc:1096: options.isolate_sources[0].Execute();
this part executes even if single-threaded.

http://codereview.chromium.org/7318002/diff/1001/src/d8.h
File src/d8.h (right):

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode220
src/d8.h:220: static Persistent<Context> CreateEvaluationContext();
renamed this to reflect that it's used in several occasions

[Søren Thygesen Gjesse, 2011-07-08 08:09:12]

Have you tried compiling this with V8 in a shared library? The use of internals
is not allowed with using shared libraries.

Maybe we should create a wrapper somewhere in the V8 API for the parts of the
internal API used for testing in the include/v8-testing.h (or add
include/v8-internal.h with the required wrappers) so we can get d8 fully
functional when building with shared libraries.

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode676
src/d8.cc:676: context_mutex_->Lock();
On 2011/07/07 16:04:49, yangguo wrote:
> mark critical section because creating a context is not thread-safe

Isn't this already handled by the use of lockers?

http://codereview.chromium.org/7318002/diff/1001/src/d8.h
File src/d8.h (right):

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode117
src/d8.h:117: SourceGroup() :
Formatting, should be ':' on new line with 4 space indent, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr....

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode118
src/d8.h:118: #if !(defined(USING_V8_SHARED) || defined(V8_SHARED))
I think we need a comment somewhere on why thid #if stuff is required. Maybe
have a global

#if !(defined(USING_V8_SHARED) || defined(V8_SHARED))
#define CAN_USE_INTERNAL_API 1
#else
#define CAN_USE_INTERNAL_API 0
#endif  // !(defined(USING_V8_SHARED) || defined(V8_SHARED))

And use that afterwards.

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode122
src/d8.h:122: #endif  // USING_V8_SHARED
#endif comment not matching #if condition.

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode139
src/d8.h:139: #endif  // USING_V8_SHARED
Ditto.

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode165
src/d8.h:165: #endif  // USING_V8_SHARED
Ditto.

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode175
src/d8.h:175: ShellOptions() :
Code style (seee
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...)

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode178
src/d8.h:178: FLAG_stress_opt(false),
Now that this is in a class I think we should drop the FLAG_ prefix.

http://codereview.chromium.org/7318002/diff/1001/src/d8.h#newcode305
src/d8.h:305: static CounterCollection* counters_;
Should these accesses to internals be protected by #if/#endif as well?

[Yang, 2011-07-08 10:03:46]

D8 contains a lot of things from v8::internal:: and apparently has never been
able to be linked to a shared V8 library. I removed the #if
(defined(USING_V8_SHARED)||defined(V8_SHARED) lines since I copied those
directly from the sample shell where it makes sense. Though note that as
consequence, the sample shell cannot handle --isolate when compiled with the
shared library.

Aside from the things I added in this CL there are a lot of other things that
would cause trouble if linked with shared library. So my plan is to land this CL
first and then, as next step, adopt your idea of creating a wrapper to expose
needed parts from the shared library so we can eventually link d8 with the
shared library.

I chose to use a mutex because using lockers in different isolates would still
result in concurrent call to Shell::CreateEvaluationContext. Even though those
concurrent calls are in different isolates, it still causes a failure when
traversing extensions when creating a context.

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7318002/diff/1001/src/d8.cc#newcode676
src/d8.cc:676: context_mutex_->Lock();
On 2011/07/08 08:09:12, Søren Gjesse wrote:
> On 2011/07/07 16:04:49, yangguo wrote:
> > mark critical section because creating a context is not thread-safe
> 
> Isn't this already handled by the use of lockers?

Unfortunately, no. As it turns out, lockers only lock inside the same isolate.
So when I have multiple threads with their own isolates respectively, there may
be concurrent accesses call to this method, causing failures. I think a mutex is
the best solution to deal with this.

[Yang, 2011-07-08 11:01:45]

minor change.

[Yang, 2011-07-08 13:15:40]

squeezed in a bug fix for http://code.google.com/p/v8/issues/detail?id=1540

http://codereview.chromium.org/7318002/diff/11001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7318002/diff/11001/src/d8.cc#newcode560
src/d8.cc:560: // Start the in-process debugger if requested.
set the listener *after* d8.js has been loaded fixes the issue when using
interactive shell with --debugger

[Søren Thygesen Gjesse, 2011-07-11 07:26:30]

LGTM