Fixing parallel execution in d8 (with -p) and some memory leaks.

src/d8.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 185 matching lines @@
 Handle<Value> Shell::Write(const Arguments& args) {
   for (int i = 0; i < args.Length(); i++) {
     HandleScope handle_scope;
     if (i != 0) {
       printf(" ");
     }
     v8::String::Utf8Value str(args[i]);
     int n = static_cast<int>(fwrite(*str, sizeof(**str), str.length(), stdout));
     if (n != str.length()) {
       printf("Error in fwrite\n");
-      exit(1);
+      Exit(1);
     }
   }
   return Undefined();
 }
 
 
 Handle<Value> Shell::EnableProfiler(const Arguments& args) {
   V8::ResumeProfiler();
   return Undefined();
 }
@@ skipping 297 matching lines @@
 }
 
 
 void Shell::MapCounters(const char* name) {
   counters_file_ = i::OS::MemoryMappedFile::create(
       name, sizeof(CounterCollection), &local_counters_);
   void* memory = (counters_file_ == NULL) ?
       NULL : counters_file_->memory();
   if (memory == NULL) {
     printf("Could not map counters file %s\n", name);
-    exit(1);
+    Exit(1);
   }
   counters_ = static_cast<CounterCollection*>(memory);
   V8::SetCounterFunction(LookupCounter);
   V8::SetCreateHistogramFunction(CreateHistogram);
   V8::SetAddHistogramSampleFunction(AddHistogramSample);
 }
 
 
 int CounterMap::Hash(const char* name) {
   int h = 0;
@@ skipping 176 matching lines @@
   return global_template;
 }
 
 
 void Shell::Initialize() {
 #ifdef COMPRESS_STARTUP_DATA_BZ2
   BZip2Decompressor startup_data_decompressor;
   int bz2_result = startup_data_decompressor.Decompress();
   if (bz2_result != BZ_OK) {
     fprintf(stderr, "bzip error code: %d\n", bz2_result);
-    exit(1);
+    Exit(1);
   }
 #endif
 
 #ifndef V8_SHARED
   Shell::counter_map_ = new CounterMap();
   // Set up counters
   if (i::StrLength(i::FLAG_map_counters) != 0)
     MapCounters(i::FLAG_map_counters);
   if (i::FLAG_dump_counters) {
     V8::SetCounterFunction(LookupCounter);
@@ skipping 41 matching lines @@
   }
   i::Handle<i::JSArray> arguments_jsarray =
       FACTORY->NewJSArrayWithElements(arguments_array);
   context->Global()->Set(String::New("arguments"),
                          Utils::ToLocal(arguments_jsarray));
 #endif  // V8_SHARED
   return context;
 }
 
 
+void Shell::Exit(int exit_code) {
+  // Use _exit instead of exit to avoid races between isolate
+  // threads and static destructors.
+  fflush(stdout);
+  fflush(stderr);
+  _exit(exit_code);
+}
+
+
 #ifndef V8_SHARED
 void Shell::OnExit() {
   if (i::FLAG_dump_counters) {
     printf("+----------------------------------------+-------------+\n");
     printf("| Name                                   | Value       |\n");
     printf("+----------------------------------------+-------------+\n");
     for (CounterMap::Iterator i(counter_map_); i.More(); i.Next()) {
       Counter* counter = i.CurrentValue();
       if (counter->is_histogram()) {
         printf("| c:%-36s | %11i |\n", i.CurrentKey(), counter->count());
@@ skipping 118 matching lines @@
     ExecuteString(String::New(buffer), name, true, true);
   }
 #endif  // V8_SHARED
   printf("\n");
 }
 
 
 #ifndef V8_SHARED
 class ShellThread : public i::Thread {
  public:
-  ShellThread(int no, i::Vector<const char> files)
+  ShellThread(int no, char* files)

[Jakob Kummerow, 2011/09/13 11:55:11]

As discussed, let's add comments à la:
// Takes ownership of |files|.

       : Thread("d8:ShellThread"),
         no_(no), files_(files) { }
+  ~ShellThread() {
+    delete[] files_;
+  }
   virtual void Run();
  private:
   int no_;
-  i::Vector<const char> files_;
+  char* files_;
 };
 
 
 void ShellThread::Run() {
-  char* ptr = const_cast<char*>(files_.start());
+  char* ptr = files_;
   while ((ptr != NULL) && (*ptr != '\0')) {
     // For each newline-separated line.
     char* next_line = ReadLine(ptr);
 
     if (*ptr == '#') {
       // Skip comment lines.
       ptr = next_line;
       continue;
     }
 
     // Prepare the context for this thread.
     Locker locker;
-    HandleScope scope;
+    HandleScope outer_scope;
     Persistent<Context> thread_context = Shell::CreateEvaluationContext();
     Context::Scope context_scope(thread_context);
 
     while ((ptr != NULL) && (*ptr != '\0')) {
+      HandleScope inner_scope;
       char* filename = ptr;
       ptr = ReadWord(ptr);
 
       // Skip empty strings.
       if (strlen(filename) == 0) {
         break;

[Jakob Kummerow, 2011/09/13 11:55:11]

s/break/continue/

       }
 
       Handle<String> str = Shell::ReadFile(filename);
       if (str.IsEmpty()) {
-        printf("WARNING: %s not found\n", filename);
-        break;
+        printf("File '%s' not found\n", filename);
+        Shell::Exit(1);
       }
 
       Shell::ExecuteString(str, String::New(filename), false, false);
     }
 
     thread_context.Dispose();
     ptr = next_line;
   }
 }
 #endif  // V8_SHARED
 
 
 SourceGroup::~SourceGroup() {
 #ifndef V8_SHARED
   delete next_semaphore_;
   next_semaphore_ = NULL;
   delete done_semaphore_;
   done_semaphore_ = NULL;
   delete thread_;
   thread_ = NULL;
 #endif  // V8_SHARED
 }
 
 
-void SourceGroup::ExitShell(int exit_code) {
-  // Use _exit instead of exit to avoid races between isolate
-  // threads and static destructors.
-  fflush(stdout);
-  fflush(stderr);
-  _exit(exit_code);
-}
-
-
 void SourceGroup::Execute() {
   for (int i = begin_offset_; i < end_offset_; ++i) {
     const char* arg = argv_[i];
     if (strcmp(arg, "-e") == 0 && i + 1 < end_offset_) {
       // Execute argument given to -e option directly.
       HandleScope handle_scope;
       Handle<String> file_name = String::New("unnamed");
       Handle<String> source = String::New(argv_[i + 1]);
       if (!Shell::ExecuteString(source, file_name, false, true)) {
-        ExitShell(1);
-        return;
+        Shell::Exit(1);
       }
       ++i;
     } else if (arg[0] == '-') {
       // Ignore other options. They have been parsed already.
     } else {
       // Use all other arguments as names of files to load and run.
       HandleScope handle_scope;
       Handle<String> file_name = String::New(arg);
       Handle<String> source = ReadFile(arg);
       if (source.IsEmpty()) {
         printf("Error reading '%s'\n", arg);
-        ExitShell(1);
-        return;
+        Shell::Exit(1);
       }
       if (!Shell::ExecuteString(source, file_name, false, true)) {
-        ExitShell(1);
-        return;
+        Shell::Exit(1);
       }
     }
   }
 }
 
 
 Handle<String> SourceGroup::ReadFile(const char* name) {
   int size;
   const char* chars = ReadChars(name, &size);
   if (chars == NULL) return Handle<String>();
@@ skipping 50 matching lines @@
   if (thread_ == NULL) return;
   if (Shell::options.last_run) {
     thread_->Join();
   } else {
     done_semaphore_->Wait();
   }
 }
 #endif  // V8_SHARED
 
 
-ShellOptions::~ShellOptions() {
-  delete[] isolate_sources;
-  isolate_sources = NULL;
-#ifndef V8_SHARED
-  delete parallel_files;
-  parallel_files = NULL;
-#endif  // V8_SHARED
-}
-
-
 bool Shell::SetOptions(int argc, char* argv[]) {
   for (int i = 0; i < argc; i++) {
     if (strcmp(argv[i], "--stress-opt") == 0) {
       options.stress_opt = true;
       argv[i] = NULL;
     } else if (strcmp(argv[i], "--stress-deopt") == 0) {
       options.stress_deopt = true;
       argv[i] = NULL;
     } else if (strcmp(argv[i], "--noalways-opt") == 0) {
       // No support for stressing if we can't use --always-opt.
@@ skipping 45 matching lines @@
     } else if (strcmp(argv[i], "-f") == 0) {
       // Ignore any -f flags for compatibility with other stand-alone
       // JavaScript engines.
       continue;
     } else if (strcmp(argv[i], "--isolate") == 0) {
 #ifdef V8_SHARED
       printf("D8 with shared library does not support multi-threading\n");
       return false;
 #endif  // V8_SHARED
       options.num_isolates++;
+    } else if (strcmp(argv[i], "-p") == 0) {
+      options.num_parallel_files++;
+#ifdef V8_SHARED
+      printf("D8 with shared library does not support multi-threading\n");
+      return false;
+#endif  // V8_SHARED
     }
 #ifdef V8_SHARED
     else if (strcmp(argv[i], "--dump-counters") == 0) {
       printf("D8 with shared library does not include counters\n");
       return false;
-    } else if (strcmp(argv[i], "-p") == 0) {
-      printf("D8 with shared library does not support multi-threading\n");
-      return false;
     } else if (strcmp(argv[i], "--debugger") == 0) {
       printf("Javascript debugger not included\n");
       return false;
     }
 #endif  // V8_SHARED
   }
 
 #ifndef V8_SHARED
   // Run parallel threads if we are not using --isolate
+  options.parallel_files = new char*[options.num_parallel_files];
+  int parallel_files_set = 0;
   for (int i = 1; i < argc; i++) {
     if (argv[i] == NULL) continue;
     if (strcmp(argv[i], "-p") == 0 && i + 1 < argc) {
       if (options.num_isolates > 1) {
         printf("-p is not compatible with --isolate\n");
         return false;
       }
       argv[i] = NULL;
-      if (options.parallel_files == NULL) {
-        options.parallel_files = new i::List<i::Vector<const char> >();
-      }
-      int size = 0;
-      const char* files = ReadChars(argv[++i], &size);
-      if (files == NULL) {
-        printf("-p option incomplete\n");
-        return false;
-      }
+      options.parallel_files[parallel_files_set++] = argv[++i];

[Jakob Kummerow, 2011/09/13 11:55:11]

I'd prefer to have the increment operations on their own lines, but since I
can't find that rule in the style guide I leave it up to you.

       argv[i] = NULL;
-      options.parallel_files->Add(i::Vector<const char>(files, size));
-      delete[] files;
     }
   }
+  if (parallel_files_set != options.num_parallel_files) {
+    printf("-p requires a file containing a list of files as parameter\n");
+    return false;
+  }
 #endif  // V8_SHARED
 
   v8::V8::SetFlagsFromCommandLine(&argc, argv, true);
 
   // Set up isolated source groups.
   options.isolate_sources = new SourceGroup[options.num_isolates];
   SourceGroup* current = options.isolate_sources;
   current->Begin(argv, 1);
   for (int i = 1; i < argc; i++) {
     const char* str = argv[i];
     if (strcmp(str, "--isolate") == 0) {
       current->End(i);
       current++;
       current->Begin(argv, i + 1);
     } else if (strncmp(argv[i], "--", 2) == 0) {
       printf("Warning: unknown flag %s.\nTry --help for options\n", argv[i]);
     }
   }
   current->End(argc);
 
   return true;
 }
 
 
 int Shell::RunMain(int argc, char* argv[]) {
 #ifndef V8_SHARED
   i::List<i::Thread*> threads(1);
-  if (options.parallel_files != NULL)
-    for (int i = 0; i < options.parallel_files->length(); i++) {
-      i::Vector<const char> files = options.parallel_files->at(i);
+  if (options.parallel_files != NULL) {
+    for (int i = 0; i < options.num_parallel_files; i++) {
+      char* files;

[Jakob Kummerow, 2011/09/13 11:55:11]

We don't like uninitialized pointers. It's safe in this case, but "char* files =
NULL;" is better.

+      { Locker lock(Isolate::GetCurrent());
+        int size;
+        files = ReadChars(options.parallel_files[i], &size);
+      }
+      if (files == NULL) {
+        printf("File list '%s' not found\n", options.parallel_files[i]);
+        Exit(1);
+      }
       ShellThread* thread = new ShellThread(threads.length(), files);
       thread->Start();
       threads.Add(thread);
     }
-
+  }

[Jakob Kummerow, 2011/09/13 11:55:11]

+1 :-)

   for (int i = 1; i < options.num_isolates; ++i) {
     options.isolate_sources[i].StartExecuteInThread();
   }
 #endif  // V8_SHARED
   {  // NOLINT
     Locker lock;
     HandleScope scope;
     Persistent<Context> context = CreateEvaluationContext();
     {
       Context::Scope cscope(context);
       options.isolate_sources[0].Execute();
     }
     if (options.last_run) {
       // Keep using the same context in the interactive shell
       evaluation_context_ = context;
     } else {
       context.Dispose();
     }
 
 #ifndef V8_SHARED
     // Start preemption if threads have been created and preemption is enabled.
-    if (options.parallel_files != NULL
-        && threads.length() > 0
+    if (threads.length() > 0
         && options.use_preemption) {
       Locker::StartPreemption(options.preemption_interval);
     }
 #endif  // V8_SHARED
   }
 
 #ifndef V8_SHARED
   for (int i = 1; i < options.num_isolates; ++i) {
     options.isolate_sources[i].WaitForThread();
   }
 
-  if (options.parallel_files != NULL)
-    for (int i = 0; i < threads.length(); i++) {
-      i::Thread* thread = threads[i];
-      thread->Join();
-      delete thread;
-    }
+  for (int i = 0; i < threads.length(); i++) {
+    i::Thread* thread = threads[i];
+    thread->Join();
+    delete thread;
+  }
+
+  if (threads.length() > 0
+      && options.use_preemption) {

[Jakob Kummerow, 2011/09/13 11:55:11]

Both conditions could go in one line, I think.

+    Locker lock;
+    Locker::StopPreemption();
+  }
 #endif  // V8_SHARED
   return 0;
 }
 
 
 int Shell::Main(int argc, char* argv[]) {
   if (!SetOptions(argc, argv)) return 1;
   Initialize();
 
   int result = 0;
@@ skipping 46 matching lines @@
 }
 
 }  // namespace v8
 
 
 #ifndef GOOGLE3
 int main(int argc, char* argv[]) {
   return v8::Shell::Main(argc, argv);
 }
 #endif