directly invoke runnable from native code #4128

Merged
merged 14 commits into from Dec 8, 2016

Conversation

Projects
None yet
4 participants
@JieGhost
Contributor

JieGhost commented Dec 7, 2016

Problem

#4024
In current engine implementation, runnable execution has 3 layers. Native scheduler generates runnable batch, sent to python scheduler, and then sent to engine.Engine class. This introduces some overhead at language boundary, like converting runnable batch from rust to python and also converting results from Python to rust. Actually the only thing that has to run in Python is the runnable itself. All other steps can happen in rust.

Solution

Create a new Python callback "invoke_runnable". Move all the engine execution logic into rust. So now native code will process runnable batch, and call invoke_runnable for each runnable object.

Future Work

We can investigate the possibility of invoking runnables concurrently from rust code. In addition, some runnable function can be ported to Rust from Python.

@stuhood

stuhood approved these changes Dec 7, 2016

Looks good! Thanks Yujie.

src/rust/engine/src/core.rs
@@ -99,6 +99,23 @@ impl Key {
}
}
+#[repr(C)]
+#[derive(Clone, Copy, Debug, Default)]
+pub struct RunnableComplete {

This comment has been minimized.

@stuhood

stuhood Dec 7, 2016

Member

These structs might be better in externs, since they're only used there?

@stuhood

stuhood Dec 7, 2016

Member

These structs might be better in externs, since they're only used there?

This comment has been minimized.

@JieGhost

JieGhost Dec 7, 2016

Contributor

done.

@JieGhost

JieGhost Dec 7, 2016

Contributor

done.

src/rust/engine/src/core.rs
+}
+
+impl RunnableComplete {
+ pub fn value(&self) -> &Value {

This comment has been minimized.

@stuhood

stuhood Dec 7, 2016

Member

You can skip making accessors if you mark the struct fields 'pub'... for simple structs like this, accessors generally seem premature, and I've been avoiding them.

pub struct RunnableComplete {
  pub value: Value,
  ..
}
@stuhood

stuhood Dec 7, 2016

Member

You can skip making accessors if you mark the struct fields 'pub'... for simple structs like this, accessors generally seem premature, and I've been avoiding them.

pub struct RunnableComplete {
  pub value: Value,
  ..
}

This comment has been minimized.

@JieGhost

JieGhost Dec 7, 2016

Contributor

done

@JieGhost

JieGhost Dec 7, 2016

Contributor

done

src/rust/engine/src/externs.rs
@@ -101,6 +105,12 @@ impl Externs {
pub fn create_exception(&self, msg: String) -> Value {
(self.create_exception)(self.context, msg.as_ptr(), msg.len() as u64)
}
+
+ pub fn invoke_runnable(&self, runnable: &Runnable) -> RunnableComplete {
+ let args_clone: Vec<Value> = runnable.args().clone();

This comment has been minimized.

@stuhood

stuhood Dec 7, 2016

Member

Is it necessary to clone here? I remember needing to in another case, but it seems like it should be fine not to here, and to just call as_ptr on the input (we know that the function we're calling won't alias it). If it's forcing you to, then nevermind.

@stuhood

stuhood Dec 7, 2016

Member

Is it necessary to clone here? I remember needing to in another case, but it seems like it should be fine not to here, and to just call as_ptr on the input (we know that the function we're calling won't alias it). If it's forcing you to, then nevermind.

This comment has been minimized.

@JieGhost

JieGhost Dec 7, 2016

Contributor

It works without "clone". Thanks!

@JieGhost

JieGhost Dec 7, 2016

Contributor

It works without "clone". Thanks!

@baroquebobcat

I like it! Ship it, modulo Stu's comments.

src/rust/engine/src/externs.rs
+
+ pub fn invoke_runnable(&self, runnable: &Runnable) -> RunnableComplete {
+ let args_clone: Vec<Value> = runnable.args().clone();
+ (self.invoke_runnable)(

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

Are the parens around self.invoke_runnable required?

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

Are the parens around self.invoke_runnable required?

This comment has been minimized.

@stuhood

stuhood Dec 7, 2016

Member

Yes: to differentiate between the struct field and the function of the same name.

@stuhood

stuhood Dec 7, 2016

Member

Yes: to differentiate between the struct field and the function of the same name.

@JieGhost JieGhost merged commit f60eefb into pantsbuild:master Dec 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

JieGhost added a commit that referenced this pull request Dec 8, 2016

directly invoke runnable from native code (#4128)
Problem

In current engine implementation, runnable execution has 3 layers.
Native scheduler generates runnable batch, sent to python scheduler,
and then sent to engine.Engine class. This introduces some overhead
at language boundary, like converting runnable batch from rust to
python and also converting results from Python to rust. Actually the
only thing that has to run in Python is the runnable itself. All other
steps can happen in rust.

Solution

Create a new Python callback "invoke_runnable". Move all the engine
execution logic into rust. So now native code will process runnable batch,
and call invoke_runnable for each runnable object.

Future Work

We can investigate the possibility of invoking runnables concurrently from
rust code. In addition, some runnable function can be ported to Rust from Python.

@JieGhost JieGhost deleted the JieGhost:yujieproject/runnable branch Jan 9, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

directly invoke runnable from native code (#4128)
Problem

In current engine implementation, runnable execution has 3 layers.
Native scheduler generates runnable batch, sent to python scheduler,
and then sent to engine.Engine class. This introduces some overhead
at language boundary, like converting runnable batch from rust to
python and also converting results from Python to rust. Actually the
only thing that has to run in Python is the runnable itself. All other
steps can happen in rust.

Solution

Create a new Python callback "invoke_runnable". Move all the engine
execution logic into rust. So now native code will process runnable batch,
and call invoke_runnable for each runnable object.

Future Work

We can investigate the possibility of invoking runnables concurrently from
rust code. In addition, some runnable function can be ported to Rust from Python.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment