removed some unnecessary stuff from d8's initialization

removed some unnecessary stuff from d8's initialization
and got rid of the utility context unless interactive shell is used

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

[Yang, 2011-06-21 02:25:55]

As previously mentioned in my mail, d8's initialization takes longer than the
sample shell simply because the debug object is loaded prematurely, unnecessary
in most cases as only few test scripts require it. The debug object is loaded
prematurely in order to assign a security token so that it becomes accessible in
the evaluation context.

Aside from a few small changes that I thought was reasonable, I made it so that
the debug object is not loaded and the security token not set unless the debug
object is exposed by an identifier to the javascript runtime (usually by command
line flag). This reduces d8's init time to almost as fast as the sample shell.

http://codereview.chromium.org/7219003/diff/1/src/d8-posix.cc
File src/d8-posix.cc (right):

http://codereview.chromium.org/7219003/diff/1/src/d8-posix.cc#newcode356
src/d8-posix.cc:356: Shell::evaluation_context()->Global(),
Since we don't always have an utility context now (unless in interactive shell),
we use the OS object in the evaluation context.

http://codereview.chromium.org/7219003/diff/1/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode503
src/d8.cc:503: 
This method is called only when using interactive shell, therefore expose debug
as $debug object in the utility context here.

On a side note: how do you access an object with an identifier such as "$debug"?

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode624
src/d8.cc:624: }
Only initialize utility context if an interactive shell is expected.

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode656
src/d8.cc:656: if (i::FLAG_expose_debug_as != NULL) {
If the debug object is not exposed, there is no point allowing access. The
premature debug->Load() is hurting d8's initialization time the most. In the
majority of the test scripts the debug object is not needed, yet this line
always loads the debug object, which takes a comparatively long time.

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

http://codereview.chromium.org/7219003/diff/1/src/d8.h#newcode132
src/d8.h:132: static void Initialize(bool shell);
Adding argument specifying whether to expect interactive shell or not. The
decides whether to prepare utility context.

[Søren Thygesen Gjesse, 2011-06-21 07:31:06]

http://codereview.chromium.org/7219003/diff/1/src/d8-posix.cc
File src/d8-posix.cc (right):

http://codereview.chromium.org/7219003/diff/1/src/d8-posix.cc#newcode354
src/d8-posix.cc:354: cons_args[1] = addition;
We have the function String::Concat in the API now, so I think we can get rid of
the cons_function all together.

http://codereview.chromium.org/7219003/diff/1/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode503
src/d8.cc:503: 
On 2011/06/21 02:25:55, yangguo wrote:
> This method is called only when using interactive shell, therefore expose
debug
> as $debug object in the utility context here.
> 
> On a side note: how do you access an object with an identifier such as
"$debug"?

$debug is a legal identifier, see d8.js where it is used.

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode599
src/d8.cc:599: void Shell::Initialize(bool shell) {
Rename shell to interactive?

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode638
src/d8.cc:638: evaluation_context_->SetSecurityToken(Undefined());
If we don't set the security token for the evaluation context can't we then
avoid checking for flag --expose-debug-as below and changing the security
context of the debugger - at lease in the case of running non-interactively?

For the interactive case we might need to do something to set the security token
to the same value in all the contexts.

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode658
src/d8.cc:658: debug->Load();
If --expose-debug-as is specified the debugger is already loaded - you could
just assert that the debugger context is there.

http://codereview.chromium.org/7219003/diff/1/src/d8.cc#newcode948
src/d8.cc:948: Initialize(run_shell);
I think it will be better to have a flag for running tests (e.g. --test-shell)
instead of relying on the --shell flag. You might otherwise want to run scripts
which relies on the utility context being loaded.

[Yang, 2011-06-22 00:44:08]

Made corrections according to your suggestions and included the change from
http://codereview.chromium.org/7193011/

I found a new problem though. I am able to start d8 with --debug-agent, but
starting another instance of d8 with --remote-debugger causes abortions with
complaints about locking. If I add locking (with Locker) to the affected
methods, the remote debugger starts, but many commands cause it to crash (for
example the "info" command). gdb shows that there are issues with String
conversion. It would be great if I can get a tip on whether my changes on d8 may
have broken the remote debugger.

Cheers,

Yang

http://codereview.chromium.org/7219003/diff/4002/src/d8-posix.cc
File src/d8-posix.cc (right):

http://codereview.chromium.org/7219003/diff/4002/src/d8-posix.cc#newcode349
src/d8-posix.cc:349: accumulator = String::Concat(accumulator, addition);
As you suggested, String::Concat instead of the detour over JS.

http://codereview.chromium.org/7219003/diff/4002/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7219003/diff/4002/src/d8.cc#newcode494
src/d8.cc:494: Context::Scope utility_scope(utility_context_);
I just went with your suggestion to not set the security token when using d8 for
test suites. that means that the security tokens are set when the utility script
is installed to make sure that the debug object is properly exposed.

http://codereview.chromium.org/7219003/diff/4002/src/d8.cc#newcode604
src/d8.cc:604: void Shell::Initialize(bool test_shell) {
Instead of renaming the argument to "interactive", i chose the opposite:
whenever we use d8 for test suites, the utility context is skipped. See below

http://codereview.chromium.org/7219003/diff/4002/src/d8.cc#newcode948
src/d8.cc:948: FLAG_interactive_shell = !FLAG_test_shell &&
Basically states: if using shell for test suites, do not use interactive shell.

http://codereview.chromium.org/7219003/diff/4002/src/d8.cc#newcode972
src/d8.cc:972: InstallUtilityScript();
The remote debugger requires stuff from d8.js, therefore it has to be installed.

http://codereview.chromium.org/7219003/diff/4002/src/d8.h
File src/d8.h (left):

http://codereview.chromium.org/7219003/diff/4002/src/d8.h#oldcode205
src/d8.h:205: static Handle<Context> utility_context() { return
utility_context_; }
there is no point keeping a method that's not in use anymore (it was necessary
for the JS-concat in d8-posix.cc)

[Yang, 2011-06-22 18:52:34]

Small fix to make sure remote-debugger works with --test option

To test that this patch indeed makes d8 faster in batch testing, enter into
bash:

time for i in {1..1000}; do ./d8 --test -e ""; done    (10.8s on my mac)
time for i in {1..1000}; do ./shell -e ""; done     (10.1s on my mac)

The first line and the third line should be almost equally fast.

Now the interesting thing is, if i use d8 without --test, therefore
unnecessarily initialize the utility context

time for i in {1..1000}; do ./d8 -e ""; done     (11.6s on my mac)

It takes slightly longer, but would of course require that extra option to
differentiate batch testing that do not require utility context and tests that
might need the utility context.

http://codereview.chromium.org/7219003/diff/6001/src/d8-debug.cc
File src/d8-debug.cc (right):

http://codereview.chromium.org/7219003/diff/6001/src/d8-debug.cc#newcode275
src/d8-debug.cc:275: Locker lock;
included from http://codereview.chromium.org/7205023/

http://codereview.chromium.org/7219003/diff/6001/src/d8-debug.cc#newcode304
src/d8-debug.cc:304: Locker lock;
ditto

http://codereview.chromium.org/7219003/diff/6001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7219003/diff/6001/src/d8.cc#newcode624
src/d8.cc:624: if (test_shell && !i::FLAG_remote_debugger) return;
carry on with initializing utility context in case remote debugger is used.

[Yang, 2011-06-23 21:45:53]

Hi,

I solved the problem with when to launch interactive shell a bit differently
now. If anything gets executed (script file or -e or -p), then we assume that
interactive shell is not launched. Unless of course if --shell is specified.
However, if --test is specified, the interactive shell is never launched.

I track whether anything is executed by passing a flag to Shell::RunMain by
reference and setting that flag when appropriate. Later in Shell::Main I use
that flag to (co-)determine whether the shell is launched.

Cheers,

Yang

PS: Maybe I should have opened a new CL for this issue :|

http://codereview.chromium.org/7219003/diff/13001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7219003/diff/13001/src/d8.cc#newcode827
src/d8.cc:827: int Shell::RunMain(int argc, char* argv[], bool* executed) {
Pass this flag by reference to track whether something is being executed. If so,
do not launch shell unless explicitly requested (by --shell).

[Søren Thygesen Gjesse, 2011-06-24 11:36:09]

LGTM

http://codereview.chromium.org/7219003/diff/13001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/7219003/diff/13001/src/d8.cc#newcode491
src/d8.cc:491: HandleScope scope;
Maybe add a comment here that when the utility context is created all three
contexts needs access to one another.

http://codereview.chromium.org/7219003/diff/13001/src/d8.cc#newcode928
src/d8.cc:928: bool FLAG_executed = false;
FLAG_executed -> executed or script_executed. It is not a command line flag.