Tasks #4

fitzgen · 2017-09-14T23:09:17Z

Builds on top of #3

r? @tschneidereit -- still very much a sketch, but it can evaluate files.

fitzgen · 2017-09-14T23:20:45Z

Also, it ended up being easier (mostly due to the futures_mpsc usage) to have a tokio reactor core per js task thread.

tschneidereit

This is a great first draft! I have a bunch of nits, but this is very close to being mergeable.

tschneidereit · 2017-09-15T13:35:49Z

Cargo.toml


 [dependencies]
+derive_is_enum_variant = "0.1.1"
+error-chain = "0.10.0"
 futures = "0.1.13"


Might as well make this 0.1.15, right?

Sure, didn't notice there had been new releases since I added this :-P

tschneidereit · 2017-09-15T13:44:46Z

src/lib.rs

+use std::sync::Arc;
+use std::thread;
+
+error_chain! {


I wonder if it'd make sense to switch to derive-error-chain.

Oo neat, I hadn't seen that before.

I think it is worth doing as a follow up.

tschneidereit · 2017-09-15T13:50:58Z

src/lib.rs

+    foreign_links {
+        Io(::std::io::Error)
+        /// An IO error.
+            ;


Is this really the way things like this get formatted? It looks quite weird to me. I wasn't able to find other examples, and the other Rust people here also haven't seen anything similar.

rustfmt is pretty bad at dealing with things inside macros.

This is error-chain's fault: it puts the #[attrs] after the Io(::std::io::Error) part. Most crates seem to do the unsugared version:

Io(::std::io::Error) #[doc = "An IO error."]

I find this harder to read than the sugared doc comments.

Another reason to switch to derive-error-chain (and that crate's docs mention this).

tschneidereit · 2017-09-15T13:52:16Z

src/lib.rs

+    }
+
+    errors {
+        /// Could not create a JavaScript runtime.


I guess these comments are needed because of #![deny(missing_docs)]? I find it somewhat unfortunate that they don't add any information whatsoever. No, I don't have any suggestions for improving them, but would like to see them removed if possible.

It is very nice to have when reading the auto-generated docs rather than the source.

Ah, I hadn't considered that.

tschneidereit · 2017-09-15T13:56:09Z

src/lib.rs

+    /// loop.
+    ///
+    /// The given `main` JavaScript file will be evaluated as the main task.
+    pub fn new<P>(main: P) -> Options


Can we move the main arg to a new builder method, e.g. file? We'll want to support directly executing scripts from strings, and creating JS tasks without any initial script to execute. The latter should probably be the default behavior if file isn't used. For now, let's just have Starling::run panic if no file was provided.

I was imagining the default file would be repl.js which drumroll implements a read-eval-print-loop.

And that the explicit_file.unwrap_or(repl_js_file) bits would be in src/bin/starling.rs, not the builder.

Ooh, I like the idea of implementing a repl that way!

I still think that we need to have the ability to create a task that just initializes a JS runtime without any source, or that takes a string buffer as the source to evaluate. However, we can always change all this later, so let's leave it as-is for now.

tschneidereit · 2017-09-15T15:14:17Z

src/lib.rs

+        &self.sync_io_pool
+    }
+
+    /// Get a handle to the thread pool for performing CPU bound background


Let's not call these "background tasks": we want to largely eliminate the notion of a main thread after all. Just "task" seems fine.

CPU bound native tasks?

I guess that works

tschneidereit · 2017-09-15T15:16:34Z

src/lib.rs

+    }
+
+    /// Send a message to the starling main thread synchronously.
+    pub fn send_sync(&self, msg: StarlingMessage) -> Result<()> {


Do we need the ability to send sync messages? I'd at least try to get away without it as long as possible. The error message below looks like it could be sent asynchronously?

tschneidereit · 2017-09-15T15:19:28Z

src/task.rs

+//!
+//! Tasks are **lightweight**. In order to facilitate let-it-fail error handling
+//! coupled with supervision hierarchies, idle tasks have little memory
+//! overhead, and near no time overhead.


I realize that much of this introductory comment is aspirational right now, but this part seems somewhat farther out than the rest in that it depends on SpiderMonkey stuff. Specifically, at the very least bug 1323066 needs to be done. Perhaps extend the comment to be explicit about that?

Sure thing.

tschneidereit · 2017-09-15T15:20:21Z

src/task.rs

+use js::rust::Runtime as JsRuntime;
+use std::fmt;
+use std::os;
+// use std::panic;


nit: remove this

Heh, woops :-P

tschneidereit · 2017-09-15T15:25:52Z

src/task.rs

+        js_file: path::PathBuf,
+    ) -> Result<(TaskHandle, thread::JoinHandle<()>)> {
+        let (send_handle, recv_handle) = oneshot::channel();
+        let starling2 = starling.clone();


Any reason this cannot be inlined in the create_main call directly? (Mainly asking because I dislike the name.)

I was trying to separate the concepts of spawning a thread and creating a task, since we intend to have them be different things down the line.

I don't fully understand why that needs the starling2 local, but given that this is very much a nit, I also don't feel I have to :)

Ah, I didn't realize you were specifically referring to starling2 -- my reply was about spawn_main vs create_main. This is because the first starling gets moved into the thread, and StarlingHandle is not Copy so we have to explicitly clone it into a new variable.

Yeah, all I meant was to not have this local and do starling.clone() inline in the function call. Clearly this isn't in any way important, though.

tschneidereit

\o/

tschneidereit · 2017-09-16T10:54:57Z

Cargo.toml

@@ -1,13 +1,34 @@
 [package]
-authors = ["Nick Fitzgerald <[email protected]>"]
+authors = ["The Starling Project Developers"]
+description = "The project that will henceforth be known as `starling`."


Forgot to mention this earlier: let's change this to "The Starling JS runtime". Not an ideal description, but I can't come up with anything better that's also concise right now. Proposals welcome!

tschneidereit · 2017-09-16T10:55:23Z

src/lib.rs

@@ -1,21 +1,386 @@
-//! The project that will henceforth be known as `starling`.
-
+//! The project that will henceforth be known as Starling.


Same thing here.

This commit sets up the infrastructure such that we can evaluate a JS file and exit zero or non-zero if the JS evals OK or throws an exception respectively.

Use the smup-smup branch of mozjs

6d9e8b4

tschneidereit requested changes Sep 15, 2017

View reviewed changes

fitzgen force-pushed the tasks branch from e5b6add to cf779db Compare September 15, 2017 18:06

fitzgen mentioned this pull request Sep 15, 2017

Derive error chain #6

Merged

tschneidereit approved these changes Sep 16, 2017

View reviewed changes

Sketch an outline for JS tasks

9d80225

This commit sets up the infrastructure such that we can evaluate a JS file and exit zero or non-zero if the JS evals OK or throws an exception respectively.

fitzgen force-pushed the tasks branch from cf779db to 9d80225 Compare September 18, 2017 17:53

fitzgen merged commit 9ffa24a into starlingjs:master Sep 18, 2017

fitzgen deleted the tasks branch September 18, 2017 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tasks #4

Tasks #4

fitzgen commented Sep 14, 2017

fitzgen commented Sep 14, 2017

tschneidereit left a comment

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

tschneidereit Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017

tschneidereit Sep 15, 2017

fitzgen Sep 15, 2017 •

edited

Loading

tschneidereit Sep 16, 2017

tschneidereit left a comment

tschneidereit Sep 16, 2017

tschneidereit Sep 16, 2017

		@@ -1,21 +1,386 @@
		//! The project that will henceforth be known as `starling`.

		//! The project that will henceforth be known as Starling.

Tasks #4

Tasks #4

Conversation

fitzgen commented Sep 14, 2017

fitzgen commented Sep 14, 2017

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen Sep 15, 2017 •

edited

Loading