core: snapshot improvements (#2052)

* Moves how snapshots are supplied to the Isolate. Previously they were
  given by Behavior::startup_data() but it was only called once at
  startup. It makes more sense (and simplifies Behavior) to pass it to the
  constructor of Isolate.
* Adds new libdeno type deno_snapshot instead of overloading
  deno_buf.
* Adds new libdeno method to delete snapshot deno_snapshot_delete().
* Renames deno_get_snapshot() to deno_snapshot_new().
* Makes StartupData hold references to snapshots. This was implicit when
  it previously held a deno_buf but is made explicit now. Note that
  include_bytes!() returns a &'static [u8] and we want to avoid
  copying that.

11 files changed, 152 insertions, 71 deletions

diff --git a/core/http_bench.rs b/core/http_bench.rs
index 37965510f..fced92bf8 100644
--- a/core/http_bench.rs
+++ b/core/http_bench.rs
@@ -99,15 +99,6 @@ pub type HttpBenchOp = dyn Future<Item = i32, Error = std::io::Error> + Send;
 struct HttpBench();
 
 impl Behavior for HttpBench {
-  fn startup_data(&mut self) -> Option<StartupData> {
-    let js_source = include_str!("http_bench.js");
-
-    Some(StartupData::Script(Script {
-      source: js_source.to_string(),
-      filename: "http_bench.js".to_string(),
-    }))
-  }
-
   fn dispatch(
     &mut self,
     control: &[u8],
@@ -168,7 +159,15 @@ fn main() {
     // TODO currently isolate.execute() must be run inside tokio, hence the
     // lazy(). It would be nice to not have that contraint. Probably requires
     // using v8::MicrotasksPolicy::kExplicit
-    let isolate = deno::Isolate::new(HttpBench());
+
+    let js_source = include_str!("http_bench.js");
+
+    let startup_data = StartupData::Script(Script {
+      source: js_source,
+      filename: "http_bench.js",
+    });
+
+    let isolate = deno::Isolate::new(startup_data, HttpBench());
 
     isolate.then(|r| {
       js_check(r);
diff --git a/core/isolate.rs b/core/isolate.rs
index a722f5a3c..88f42be0e 100644
--- a/core/isolate.rs
+++ b/core/isolate.rs
@@ -8,6 +8,8 @@ use crate::js_errors::JSError;
 use crate::libdeno;
 use crate::libdeno::deno_buf;
 use crate::libdeno::deno_mod;
+use crate::libdeno::Snapshot1;
+use crate::libdeno::Snapshot2;
 use crate::shared_queue::SharedQueue;
 use crate::shared_queue::RECOMMENDED_SIZE;
 use futures::Async;
@@ -16,6 +18,7 @@ use futures::Poll;
 use libc::c_void;
 use std::ffi::CStr;
 use std::ffi::CString;
+use std::ptr::null;
 use std::sync::{Arc, Mutex, Once, ONCE_INIT};
 
 pub type Buf = Box<[u8]>;
@@ -48,26 +51,24 @@ impl Future for PendingOp {
 }
 
 /// Stores a script used to initalize a Isolate
-pub struct Script {
-  pub source: String,
-  pub filename: String,
+pub struct Script<'a> {
+  pub source: &'a str,
+  pub filename: &'a str,
 }
 
 /// Represents data used to initialize isolate at startup
 /// either a binary snapshot or a javascript source file
 /// in the form of the StartupScript struct.
-pub enum StartupData {
-  Script(Script),
-  Snapshot(deno_buf),
+pub enum StartupData<'a> {
+  Script(Script<'a>),
+  Snapshot(&'a [u8]),
+  None,
 }
 
 /// Defines the behavior of an Isolate.
+// TODO(ry) Now that Behavior only has the dispatch method, it should be renamed
+// to Dispatcher.
 pub trait Behavior {
-  /// Allow for a behavior to define the snapshot or script used at
-  /// startup to initalize the isolate. Called exactly once when an
-  /// Isolate is created.
-  fn startup_data(&mut self) -> Option<StartupData>;
-
   /// Called whenever Deno.core.dispatch() is called in JavaScript. zero_copy_buf
   /// corresponds to the second argument of Deno.core.dispatch().
   fn dispatch(
@@ -109,7 +110,9 @@ impl<B: Behavior> Drop for Isolate<B> {
 static DENO_INIT: Once = ONCE_INIT;
 
 impl<B: Behavior> Isolate<B> {
-  pub fn new(mut behavior: B) -> Self {
+  /// startup_data defines the snapshot or script used at startup to initalize
+  /// the isolate.
+  pub fn new(startup_data: StartupData, behavior: B) -> Self {
     DENO_INIT.call_once(|| {
       unsafe { libdeno::deno_init() };
     });
@@ -118,16 +121,16 @@ impl<B: Behavior> Isolate<B> {
 
     let needs_init = true;
     // Seperate into Option values for eatch startup type
-    let (startup_snapshot, startup_script) = match behavior.startup_data() {
-      Some(StartupData::Snapshot(d)) => (Some(d), None),
-      Some(StartupData::Script(d)) => (None, Some(d)),
-      None => (None, None),
+    let (startup_snapshot, startup_script) = match startup_data {
+      StartupData::Snapshot(d) => (Some(d), None),
+      StartupData::Script(d) => (None, Some(d)),
+      StartupData::None => (None, None),
     };
     let config = libdeno::deno_config {
       will_snapshot: 0,
       load_snapshot: match startup_snapshot {
-        Some(s) => s,
-        None => libdeno::deno_buf::empty(),
+        Some(s) => Snapshot2::from(s),
+        None => Snapshot2::empty(),
       },
       shared: shared.as_deno_buf(),
       recv_cb: Self::pre_dispatch,
@@ -146,9 +149,7 @@ impl<B: Behavior> Isolate<B> {
 
     // If we want to use execute this has to happen here sadly.
     if let Some(s) = startup_script {
-      core_isolate
-        .execute(s.filename.as_str(), s.source.as_str())
-        .unwrap()
+      core_isolate.execute(s.filename, s.source).unwrap()
     };
 
     core_isolate
@@ -329,6 +330,18 @@ impl<B: Behavior> Isolate<B> {
     }
     out
   }
+
+  pub fn snapshot_new(&self) -> Result<Snapshot1, JSError> {
+    let snapshot = unsafe { libdeno::deno_snapshot_new(self.libdeno_isolate) };
+    if let Some(js_error) = self.last_exception() {
+      assert_eq!(snapshot.data_ptr, null());
+      assert_eq!(snapshot.data_len, 0);
+      return Err(js_error);
+    }
+    assert_ne!(snapshot.data_ptr, null());
+    assert_ne!(snapshot.data_len, 0);
+    Ok(snapshot)
+  }
 }
 
 /// Called during mod_instantiate() to resolve imports.
@@ -546,10 +559,13 @@ pub mod tests {
 
   impl TestBehavior {
     pub fn setup(mode: TestBehaviorMode) -> Isolate<Self> {
-      let mut isolate = Isolate::new(TestBehavior {
-        dispatch_count: 0,
-        mode,
-      });
+      let mut isolate = Isolate::new(
+        StartupData::None,
+        TestBehavior {
+          dispatch_count: 0,
+          mode,
+        },
+      );
       js_check(isolate.execute(
         "setup.js",
         r#"
@@ -566,10 +582,6 @@ pub mod tests {
   }
 
   impl Behavior for TestBehavior {
-    fn startup_data(&mut self) -> Option<StartupData> {
-      None
-    }
-
     fn dispatch(
       &mut self,
       control: &[u8],
diff --git a/core/libdeno.rs b/core/libdeno.rs
index 1a80330de..e6445f299 100644
--- a/core/libdeno.rs
+++ b/core/libdeno.rs
@@ -4,6 +4,7 @@ use libc::c_char;
 use libc::c_int;
 use libc::c_void;
 use libc::size_t;
+use std::marker::PhantomData;
 use std::ops::{Deref, DerefMut};
 use std::ptr::null;
 
@@ -108,6 +109,56 @@ impl AsMut<[u8]> for deno_buf {
   }
 }
 
+#[repr(C)]
+pub struct deno_snapshot<'a> {
+  pub data_ptr: *const u8,
+  pub data_len: usize,
+  _marker: PhantomData<&'a [u8]>,
+}
+
+/// `deno_snapshot` can not clone, and there is no interior mutability.
+/// This type satisfies Send bound.
+unsafe impl Send for deno_snapshot<'_> {}
+
+// TODO(ry) Snapshot1 and Snapshot2 are not very good names and need to be
+// reconsidered. The entire snapshotting interface is still under construction.
+
+/// The type returned from deno_snapshot_new. Needs to be dropped.
+pub type Snapshot1<'a> = deno_snapshot<'a>;
+
+// TODO Does this make sense?
+impl Drop for Snapshot1<'_> {
+  fn drop(&mut self) {
+    unsafe { deno_snapshot_delete(self) }
+  }
+}
+
+/// The type created from slice. Used for loading.
+pub type Snapshot2<'a> = deno_snapshot<'a>;
+
+/// Converts Rust &Buf to libdeno `deno_buf`.
+impl<'a> From<&'a [u8]> for Snapshot2<'a> {
+  #[inline]
+  fn from(x: &'a [u8]) -> Self {
+    Self {
+      data_ptr: x.as_ref().as_ptr(),
+      data_len: x.len(),
+      _marker: PhantomData,
+    }
+  }
+}
+
+impl Snapshot2<'_> {
+  #[inline]
+  pub fn empty() -> Self {
+    Self {
+      data_ptr: null(),
+      data_len: 0,
+      _marker: PhantomData,
+    }
+  }
+}
+
 #[allow(non_camel_case_types)]
 type deno_recv_cb = unsafe extern "C" fn(
   user_data: *mut c_void,
@@ -126,9 +177,9 @@ type deno_resolve_cb = unsafe extern "C" fn(
 ) -> deno_mod;
 
 #[repr(C)]
-pub struct deno_config {
+pub struct deno_config<'a> {
   pub will_snapshot: c_int,
-  pub load_snapshot: deno_buf,
+  pub load_snapshot: Snapshot2<'a>,
   pub shared: deno_buf,
   pub recv_cb: deno_recv_cb,
 }
@@ -214,4 +265,9 @@ extern "C" {
     user_data: *const c_void,
     id: deno_mod,
   );
+
+  pub fn deno_snapshot_new(i: *const isolate) -> Snapshot1<'static>;
+
+  #[allow(dead_code)]
+  pub fn deno_snapshot_delete(s: &mut deno_snapshot);
 }
diff --git a/core/libdeno/api.cc b/core/libdeno/api.cc
index fa1fe92f9..ad9c2a574 100644
--- a/core/libdeno/api.cc
+++ b/core/libdeno/api.cc
@@ -93,7 +93,7 @@ void deno_unlock(Deno* d_) {
   d->locker_ = nullptr;
 }
 
-deno_buf deno_get_snapshot(Deno* d_) {
+deno_snapshot deno_snapshot_new(Deno* d_) {
   auto* d = unwrap(d_);
   CHECK_NOT_NULL(d->snapshot_creator_);
   d->ClearModules();
@@ -101,8 +101,12 @@ deno_buf deno_get_snapshot(Deno* d_) {
 
   auto blob = d->snapshot_creator_->CreateBlob(
       v8::SnapshotCreator::FunctionCodeHandling::kKeep);
-  return {nullptr, 0, reinterpret_cast<uint8_t*>(const_cast<char*>(blob.data)),
-          blob.raw_size, 0};
+  return {reinterpret_cast<uint8_t*>(const_cast<char*>(blob.data)),
+          blob.raw_size};
+}
+
+void deno_snapshot_delete(deno_snapshot snapshot) {
+  delete[] snapshot.data_ptr;
 }
 
 static std::unique_ptr<v8::Platform> platform;
diff --git a/core/libdeno/deno.h b/core/libdeno/deno.h
index f3902985e..8054090ed 100644
--- a/core/libdeno/deno.h
+++ b/core/libdeno/deno.h
@@ -18,6 +18,11 @@ typedef struct {
   size_t zero_copy_id;  // 0 = normal, 1 = must call deno_zero_copy_release.
 } deno_buf;
 
+typedef struct {
+  uint8_t* data_ptr;
+  size_t data_len;
+} deno_snapshot;
+
 typedef struct deno_s Deno;
 
 // A callback to receive a message from a libdeno.send() javascript call.
@@ -31,22 +36,27 @@ const char* deno_v8_version();
 void deno_set_v8_flags(int* argc, char** argv);
 
 typedef struct {
-  int will_snapshot;       // Default 0. If calling deno_get_snapshot 1.
-  deno_buf load_snapshot;  // Optionally: A deno_buf from deno_get_snapshot.
-  deno_buf shared;         // Shared buffer to be mapped to libdeno.shared
-  deno_recv_cb recv_cb;    // Maps to libdeno.send() calls.
+  int will_snapshot;            // Default 0. If calling deno_snapshot_new 1.
+  deno_snapshot load_snapshot;  // A startup snapshot to use.
+  deno_buf shared;              // Shared buffer to be mapped to libdeno.shared
+  deno_recv_cb recv_cb;         // Maps to libdeno.send() calls.
 } deno_config;
 
 // Create a new deno isolate.
-// Warning: If config.will_snapshot is set, deno_get_snapshot() must be called
+// Warning: If config.will_snapshot is set, deno_snapshot_new() must be called
 // or an error will result.
 Deno* deno_new(deno_config config);
+void deno_delete(Deno* d);
 
-// Generate a snapshot. The resulting buf can be used with deno_new.
-// The caller must free the returned data by calling delete[] buf.data_ptr.
-deno_buf deno_get_snapshot(Deno* d);
+// Generate a snapshot. The resulting buf can be used in as the load_snapshot
+// member in deno_confg.
+// When calling this function, the caller must have created the isolate "d" with
+// "will_snapshot" set to 1.
+// The caller must free the returned data with deno_snapshot_delete().
+deno_snapshot deno_snapshot_new(Deno* d);
 
-void deno_delete(Deno* d);
+// Only for use with data returned from deno_snapshot_new.
+void deno_snapshot_delete(deno_snapshot);
 
 void deno_lock(Deno* d);
 void deno_unlock(Deno* d);
diff --git a/core/libdeno/internal.h b/core/libdeno/internal.h
index 71cf731b6..ce8c63c56 100644
--- a/core/libdeno/internal.h
+++ b/core/libdeno/internal.h
@@ -169,6 +169,7 @@ static intptr_t external_references[] = {
     0};
 
 static const deno_buf empty_buf = {nullptr, 0, nullptr, 0, 0};
+static const deno_snapshot empty_snapshot = {nullptr, 0};
 
 Deno* NewFromSnapshot(void* user_data, deno_recv_cb cb);
 
diff --git a/core/libdeno/libdeno_test.cc b/core/libdeno/libdeno_test.cc
index 8949e7f4a..7b936cc32 100644
--- a/core/libdeno/libdeno_test.cc
+++ b/core/libdeno/libdeno_test.cc
@@ -10,10 +10,10 @@ TEST(LibDenoTest, InitializesCorrectly) {
 }
 
 TEST(LibDenoTest, Snapshotter) {
-  Deno* d1 = deno_new(deno_config{1, empty, empty, nullptr});
+  Deno* d1 = deno_new(deno_config{1, empty_snapshot, empty, nullptr});
   deno_execute(d1, nullptr, "a.js", "a = 1 + 2");
   EXPECT_EQ(nullptr, deno_last_exception(d1));
-  deno_buf test_snapshot = deno_get_snapshot(d1);
+  deno_snapshot test_snapshot = deno_snapshot_new(d1);
   deno_delete(d1);
 
   Deno* d2 = deno_new(deno_config{0, test_snapshot, empty, nullptr});
@@ -21,7 +21,7 @@ TEST(LibDenoTest, Snapshotter) {
   EXPECT_EQ(nullptr, deno_last_exception(d2));
   deno_delete(d2);
 
-  delete[] test_snapshot.data_ptr;
+  deno_snapshot_delete(test_snapshot);
 }
 
 TEST(LibDenoTest, CanCallFunction) {
@@ -249,7 +249,7 @@ TEST(LibDenoTest, CheckPromiseErrors) {
 }
 
 TEST(LibDenoTest, LastException) {
-  Deno* d = deno_new(deno_config{0, empty, empty, nullptr});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, nullptr});
   EXPECT_EQ(deno_last_exception(d), nullptr);
   deno_execute(d, nullptr, "a.js", "\n\nthrow Error('boo');\n\n");
   EXPECT_STREQ(deno_last_exception(d),
@@ -264,7 +264,7 @@ TEST(LibDenoTest, LastException) {
 }
 
 TEST(LibDenoTest, EncodeErrorBug) {
-  Deno* d = deno_new(deno_config{0, empty, empty, nullptr});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, nullptr});
   EXPECT_EQ(deno_last_exception(d), nullptr);
   deno_execute(d, nullptr, "a.js", "eval('a')");
   EXPECT_STREQ(
@@ -293,7 +293,7 @@ TEST(LibDenoTest, Shared) {
 }
 
 TEST(LibDenoTest, Utf8Bug) {
-  Deno* d = deno_new(deno_config{0, empty, empty, nullptr});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, nullptr});
   // The following is a valid UTF-8 javascript which just defines a string
   // literal. We had a bug where libdeno would choke on this.
   deno_execute(d, nullptr, "a.js", "x = \"\xEF\xBF\xBD\"");
@@ -318,7 +318,7 @@ TEST(LibDenoTest, LibDenoEvalContextError) {
 TEST(LibDenoTest, SharedAtomics) {
   int32_t s[] = {0, 1, 2};
   deno_buf shared = {nullptr, 0, reinterpret_cast<uint8_t*>(s), sizeof s, 0};
-  Deno* d = deno_new(deno_config{0, empty, shared, nullptr});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, shared, nullptr});
   deno_execute(d, nullptr, "a.js",
                "Atomics.add(new Int32Array(Deno.core.shared), 0, 1)");
   EXPECT_EQ(nullptr, deno_last_exception(d));
diff --git a/core/libdeno/modules_test.cc b/core/libdeno/modules_test.cc
index 9f9228430..0eaa9e8eb 100644
--- a/core/libdeno/modules_test.cc
+++ b/core/libdeno/modules_test.cc
@@ -13,7 +13,7 @@ void recv_cb(void* user_data, deno_buf buf, deno_buf zero_copy_buf) {
 
 TEST(ModulesTest, Resolution) {
   exec_count = 0;  // Reset
-  Deno* d = deno_new(deno_config{0, empty, empty, recv_cb});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, recv_cb});
   EXPECT_EQ(0, exec_count);
 
   static deno_mod a = deno_mod_new(d, true, "a.js",
@@ -66,7 +66,7 @@ TEST(ModulesTest, Resolution) {
 
 TEST(ModulesTest, ResolutionError) {
   exec_count = 0;  // Reset
-  Deno* d = deno_new(deno_config{0, empty, empty, recv_cb});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, recv_cb});
   EXPECT_EQ(0, exec_count);
 
   static deno_mod a = deno_mod_new(d, true, "a.js",
@@ -99,7 +99,7 @@ TEST(ModulesTest, ResolutionError) {
 
 TEST(ModulesTest, ImportMetaUrl) {
   exec_count = 0;  // Reset
-  Deno* d = deno_new(deno_config{0, empty, empty, recv_cb});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, recv_cb});
   EXPECT_EQ(0, exec_count);
 
   static deno_mod a =
@@ -119,7 +119,7 @@ TEST(ModulesTest, ImportMetaUrl) {
 }
 
 TEST(ModulesTest, ImportMetaMain) {
-  Deno* d = deno_new(deno_config{0, empty, empty, recv_cb});
+  Deno* d = deno_new(deno_config{0, empty_snapshot, empty, recv_cb});
 
   const char* throw_not_main_src = "if (!import.meta.main) throw 'err'";
   static deno_mod throw_not_main =
diff --git a/core/libdeno/snapshot_creator.cc b/core/libdeno/snapshot_creator.cc
index 19098392d..081bd1156 100644
--- a/core/libdeno/snapshot_creator.cc
+++ b/core/libdeno/snapshot_creator.cc
@@ -8,8 +8,6 @@
 #include "third_party/v8/include/v8.h"
 #include "third_party/v8/src/base/logging.h"
 
-namespace deno {}  // namespace deno
-
 int main(int argc, char** argv) {
   const char* snapshot_out_bin = argv[1];
   const char* js_fn = argv[2];
@@ -23,7 +21,7 @@ int main(int argc, char** argv) {
   CHECK(deno::ReadFileToString(js_fn, &js_source));
 
   deno_init();
-  deno_config config = {1, deno::empty_buf, deno::empty_buf, nullptr};
+  deno_config config = {1, deno::empty_snapshot, deno::empty_buf, nullptr};
   Deno* d = deno_new(config);
 
   deno_execute(d, nullptr, js_fn, js_source.c_str());
@@ -34,13 +32,13 @@ int main(int argc, char** argv) {
     return 1;
   }
 
-  auto snapshot = deno_get_snapshot(d);
+  auto snapshot = deno_snapshot_new(d);
 
   std::ofstream file_(snapshot_out_bin, std::ios::binary);
   file_.write(reinterpret_cast<char*>(snapshot.data_ptr), snapshot.data_len);
   file_.close();
 
-  delete[] snapshot.data_ptr;
+  deno_snapshot_delete(snapshot);
   deno_delete(d);
 
   return file_.bad();
diff --git a/core/libdeno/test.cc b/core/libdeno/test.cc
index 1340fe8c3..3d022c904 100644
--- a/core/libdeno/test.cc
+++ b/core/libdeno/test.cc
@@ -3,7 +3,7 @@
 #include <string>
 #include "file_util.h"
 
-deno_buf snapshot = {nullptr, 0, nullptr, 0, 0};
+deno_snapshot snapshot = {nullptr, 0};
 
 int main(int argc, char** argv) {
   // Locate the snapshot.
diff --git a/core/libdeno/test.h b/core/libdeno/test.h
index 2f7c32384..dd5dc99b2 100644
--- a/core/libdeno/test.h
+++ b/core/libdeno/test.h
@@ -5,7 +5,8 @@
 #include "deno.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-extern deno_buf snapshot;  // Loaded in libdeno/test.cc
+extern deno_snapshot snapshot;  // Loaded in libdeno/test.cc
 const deno_buf empty = {nullptr, 0, nullptr, 0, 0};
+const deno_snapshot empty_snapshot = {nullptr, 0};
 
 #endif  // TEST_H_