Use the ThreadPool for all isolates and native ports. Previously,

runtime/lib/isolate.cc

Index: runtime/lib/isolate.cc
===================================================================
--- runtime/lib/isolate.cc	(revision 6423)
+++ runtime/lib/isolate.cc	(working copy)
@@ -10,7 +10,7 @@
 #include "vm/dart_entry.h"
 #include "vm/exceptions.h"
 #include "vm/longjump.h"
-#include "vm/message.h"
+#include "vm/message_handler.h"
 #include "vm/object.h"
 #include "vm/object_store.h"
 #include "vm/port.h"
@@ -53,14 +53,11 @@
 }
 
 
-// TODO(turnidge): Taking down the whole vm when an isolate fails is
-// bad.  Change this.
-static void ProcessError(const Object& obj) {
+static void StoreError(Isolate* isolate, const Object& obj) {
   ASSERT(obj.IsError());
   Error& error = Error::Handle();
   error ^= obj.raw();
-  OS::PrintErr("%s\n", error.ToErrorCString());
-  exit(255);
+  isolate->object_store()->set_sticky_error(error);
 }
 
 
@@ -113,7 +110,7 @@
 }
 
 
-static void RunIsolate(uword parameter) {
+static bool RunIsolate(uword parameter) {
   IsolateStartData* data = reinterpret_cast<IsolateStartData*>(parameter);
   Isolate* isolate = data->isolate_;
   char* library_url = data->library_url_;
@@ -121,23 +118,19 @@
   intptr_t port_id = data->port_id_;
   delete data;
 
-  Isolate::SetCurrent(isolate);
-  // Intialize stack limit in case we are running isolate in a
-  // different thread than in which it was initialized.
-  isolate->SetStackLimitFromCurrentTOS(reinterpret_cast<uword>(&isolate));
-  LongJump* base = isolate->long_jump_base();
-  LongJump jump;
-  isolate->set_long_jump_base(&jump);
-  if (setjmp(*jump.Set()) == 0) {
+  {
+    SetIsolateScope set_scope(isolate);
     Zone zone(isolate);
     HandleScope handle_scope(isolate);
     ASSERT(ClassFinalizer::FinalizePendingClasses());
     // Lookup the target class by name, create an instance and call the run
     // method.
     const String& lib_name = String::Handle(String::NewSymbol(library_url));
+    free(library_url);
     const Library& lib = Library::Handle(Library::LookupLibrary(lib_name));
     ASSERT(!lib.IsNull());
     const String& cls_name = String::Handle(String::NewSymbol(class_name));
+    free(class_name);
     const Class& target_class = Class::Handle(lib.LookupClass(cls_name));
     // TODO(iposva): Deserialize or call the constructor after allocating.
     // For now, we only support a non-parameterized or raw target class.

[siva, 2012/04/14 00:29:53]

Is this comment still valid considering the new isolate invocation APIs

[turnidge, 2012/04/17 23:46:55]

RunIsolate implements the old isolate api.  The new isolate api uses
RunIsolate2.

@@ -158,7 +151,8 @@
                                        arguments,
                                        kNoArgumentNames);
       if (result.IsError()) {
-        ProcessError(result);
+        StoreError(isolate, result);
+        return false;
       }
       ASSERT(result.IsNull());
     }
@@ -171,7 +165,7 @@
     // TODO(iposva): Allocate the proper port number here.
     const Object& local_port = Object::Handle(ReceivePortCreate(port_id));
     if (local_port.IsError()) {
-      ProcessError(local_port);
+      StoreError(isolate, local_port);

[siva, 2012/04/14 00:29:53]

need a "return false"; here?
We don't want to invoke the target function after an error has occurred.

[turnidge, 2012/04/17 23:46:55]

Done.

     }
     GrowableArray<const Object*> arguments(1);
     arguments.Add(&local_port);
@@ -181,28 +175,12 @@
                                       arguments,
                                       kNoArgumentNames);
     if (result.IsError()) {
-      ProcessError(result);
+      StoreError(isolate, result);
+      return false;
     }
     ASSERT(result.IsNull());
-    free(class_name);
-    free(library_url);
-    result = isolate->StandardRunLoop();
-    if (result.IsError()) {
-      ProcessError(result);
-    }
-    ASSERT(result.IsNull());
-
-  } else {
-    Zone zone(isolate);
-    HandleScope handle_scope(isolate);
-    const Error& error = Error::Handle(
-        Isolate::Current()->object_store()->sticky_error());
-    const char* errmsg = error.ToErrorCString();
-    OS::PrintErr("%s\n", errmsg);
-    exit(255);
   }
-  isolate->set_long_jump_base(base);
-  Dart::ShutdownIsolate();
+  return true;

[siva, 2012/04/14 00:29:53]

In the old code if the new isolate had some errors during invocation (e.g: class
not found or script compilation error etc.) it was shutting itself down. Where
does that happen in the new scenario?

[turnidge, 2012/04/17 23:46:55]

Okay.  Thanks, I had that all wrong.  I made some major changes to the CL to
handle termination correctly.  I have dropped RunBlocking and I now pass an
end_callback to Run() so that each time we run an isolate on the thread pool we
can specify what it should do on termination.

 }
 
 
@@ -297,10 +275,9 @@
                                strdup(library_url),
                                strdup(class_name),
                                port_id));
-      int result = Thread::Start(RunIsolate, data);
-      if (result != 0) {
-        FATAL1("Failed to start isolate thread %d", result);
-      }
+      Isolate::SetCurrent(NULL);
+      spawned_isolate->message_handler()->Run(
+          Dart::thread_pool(), RunIsolate, data);
     } else {
       // Error spawning the isolate, maybe due to initialization errors or
       // errors while loading the application into spawned isolate, shut
@@ -425,10 +402,8 @@
   }
 
   void Cleanup() {
-    Isolate* saved = Isolate::Current();
-    Isolate::SetCurrent(isolate());
+    SetIsolateScope set_scope(isolate());
     Dart::ShutdownIsolate();
-    Isolate::SetCurrent(saved);
   }
 
  private:
@@ -479,16 +454,11 @@
 }
 
 
-static void RunIsolate2(uword parameter) {
+static bool RunIsolate2(uword parameter) {
   SpawnState* state = reinterpret_cast<SpawnState*>(parameter);
   Isolate* isolate = state->isolate();
-
-  Isolate::SetCurrent(isolate);
-  // Intialize stack limit in case we are running isolate in a
-  // different thread than in which it was initialized.
-  isolate->SetStackLimitFromCurrentTOS(reinterpret_cast<uword>(&isolate));
-
   {
+    SetIsolateScope set_scope(isolate);
     Zone zone(isolate);
     HandleScope handle_scope(isolate);
     ASSERT(ClassFinalizer::FinalizePendingClasses());
@@ -503,16 +473,11 @@
     const Array& kNoArgNames = Array::Handle();
     result = DartEntry::InvokeStatic(func, args, kNoArgNames);
     if (result.IsError()) {
-      ProcessError(result);
+      StoreError(isolate, result);
+      return false;
     }
-
-    result = isolate->StandardRunLoop();
-    if (result.IsError()) {
-      ProcessError(result);
-    }
-    ASSERT(result.IsNull());
   }
-  Dart::ShutdownIsolate();
+  return true;
 }
 
 
@@ -551,7 +516,10 @@
   }
 
   // Start the new isolate.
-  int result = Thread::Start(RunIsolate2, reinterpret_cast<uword>(state));
+  // int result = Thread::Start(RunIsolate2, reinterpret_cast<uword>(state));
+  state->isolate()->message_handler()->Run(
+      Dart::thread_pool(), RunIsolate2, reinterpret_cast<uword>(state));
+  int result = 0;
   if (result != 0) {

[siva, 2012/04/14 00:29:53]

result has just been initialized to 0, how can it be non zero, is the check
needed?

[turnidge, 2012/04/17 23:46:55]

The check is no longer needed.  It is now a FATAL in thread_pool.cc.

Do you think that is too severe?  I could restore the old behavior by adding a
return value to ThreadPool::Run()...

[siva, 2012/04/18 22:05:00]

FATAL is fine for now, we could revisit once the new code has baked for some
time.

On 2012/04/17 23:46:55, turnidge wrote:

     const String& msg = String::Handle(String::NewFormatted(
         "Failed to start thread for isolate '%s'.  Error code '%d'.",