Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP a prototype crate design for ops #3025

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions op_hello/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# This is an imagined crate directory structure for Deno ops.

[package]
name = "op_hello"
version = "0.1.0"
authors = ["Ryan Dahl <ry@tinyclouds.org>"]
edition = "2018"

# Note: I am explicitly attempting to not have a build.rs file. I think it might
# be unnecessary for each op crate to implement their own. Then again, it might
# be unavoidable... Let's try without first.

[dependencies]
deno = "0.19.0"
# deno_std is needed because hello_test.ts depends on the "testing/mod.ts" file
# inside of deno_std.
# TODO deno_std is not yet published as crate
deno_std = "0.19.0"

12 changes: 12 additions & 0 deletions op_hello/src/hello.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const OP_HELLO: number = Deno.ops.add("hello");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is cool and I'd really like to use it that way, however due to quirks related to shared_queue I was unable to make this solution work - see comments

Copy link
Member

@bartlomieju bartlomieju Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm gonna try to implement it once more, I'll get back to you.

EDIT: Okay so the problem is that during snapshotting we're just loading JS files. The "state" used for snapshot is bare minimum and it does not have any ops registered - that means that during snapshotting Deno.ops.add("hello") wouldn't find counterpart of this op in Rust and won't be able to retrieve id.

Opinions @ry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok - well let's not worry about the exact spelling of this - we can always adjust it in the future. How about this API:

export function hello() {
  Deno.core.send(Deno.ops["hello"]);
}

https://github.com/ry/deno/blob/op_hello/op_hello_js/hello.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ry that will work (given that we "synchronize" ops on runtime initialization), however presented solution brings back @afinch7's concerns about performance hit caused by object lookup on each op call. Still we can start with this API and then figure out how to make it fast

Copy link
Member Author

@ry ry Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite sure we're not at the level where a hash look up like that would effect our performance. I hope we are some day...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I update #3002 to use this API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're in agreement, yea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ry PR updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight I think that hash lookup is fine, and I think my testing was flawed when I originally tested performance with a similar api.


// https://www.typescriptlang.org/docs/handbook/namespaces.html#splitting-across-files
namespace Deno {
/**
* The typedoc here ideally would be presevered automatically in
* lib.deno_runtime.d.ts
*/
export function hello() {
Deno.core.send(OP_HELLO);
}
}
34 changes: 34 additions & 0 deletions op_hello/src/hello_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// We need some way to import test modules.
// Attempt one:
//
// import { test } from "../../js/test_util.ts";
//
// Here it is referencing files across crate boundaries, which will break
// 'cargo package' and means the crate is not useable outside the deno tree.
// This might be okay for a first pass, but it's not the best solution.
//
// Attempt two:
// we invent a new URL for referencing files in other crates.
// this is magic and not browser compatible.. Browser compatibility for
// ops is not so important.
//
// import { test } from "crate://deno_std@0.19.0/testing/mod.ts";
//
// This is quite nice. But the version of deno_std already specified in
// Cargo.toml. I think we shouldn't repeat it.
import { test } from "crate://deno_std/testing/mod.ts";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// we invent a new URL for referencing files in other crates.
// this is magic and not browser compatible.. Browser compatibility for
// ops is not so important.
//
// import { test } from "crate://deno_std@0.19.0/testing/mod.ts";
//
// This is quite nice. But the version of deno_std already specified in
// Cargo.toml. I think we shouldn't repeat it.

This is least magical that we can hope for and it's an URL after all. I like this idea very much 👍


// If we don't do the //src reorg that I've proposed in #3022, then we might be
// able to have a very elegant URL some day using the deno crate.
//
// import { test } from "crate://deno/std/testing/mod.ts";

import "./hello.ts";

test("hello test", () => {
Deno.hello();
});

test("hello test2", () => {
Deno.hello();
});
41 changes: 41 additions & 0 deletions op_hello/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
extern crate deno;
extern crate deno_std;
use deno::*;

pub fn init(&mut isolate: Isolate) -> Result<(), ErrBox> {
isolate.register_op("hello", op_hello); // register_op defined by #3002

// Explicitly link the deno_std crate so it can be used in hello_test.ts
// Its usage looks like this:
//
// import { test } from "crate://deno_std/testing/mod.ts";
//
// In the future it might make sense to automate this function away, but I think
// it would be prudent to make the crate URL resolution as obvious as
// possible.
isolate.register_crate_url("deno_std", deno_std::get_file);

// TODO The ability to run typescript doesn't exist in deno core.
isolate.run("src/hello.ts")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about ES modules? AFAIK Isolate has only low level API for them and all the complexity is hidden in modules.rs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we should support that too.

}

fn op_hello(_control_buf: &[u8], _zero_copy_buf: Option<PinnedBuf>) -> CoreOp {
println!("Hello world");
CoreOp::Sync(Box::new([]))
}

#[test]
fn rust_test() {
match op_hello() {
CoreOp::Sync(buf) => {
assert_eq!(buf.len(), 0);
}
CoreOp::Async(_) => unreachable!(),
}
}

#[test]
fn js_test() {
// This should execute src/hello_test.ts
deno_test();
}
14 changes: 14 additions & 0 deletions op_hello_js/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# This is an imagined crate directory structure for Deno ops.
# Same as op_hello but done with JS and not TS.

[package]
name = "op_hello_js"
version = "0.1.0"
authors = ["Ryan Dahl <ry@tinyclouds.org>"]
edition = "2018"

[lib]
path = "lib.rs"

[dependencies]
deno = "0.19.0"
6 changes: 6 additions & 0 deletions op_hello_js/hello.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// TODO In the future maybe we can extract the op id in the top-level and use a
// constant. But currently it's causing problems with snapshotting.

export function hello() {
Deno.core.send(Deno.ops["hello"]);
}
4 changes: 4 additions & 0 deletions op_hello_js/hello_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// As opposed to the TypeScript op_hello example, here we want to not rely on
// deno_std, as it uses TypeScript. So here don't use a test runner.
import { hello } from "./hello.js";
hello();
27 changes: 27 additions & 0 deletions op_hello_js/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
extern crate deno;
use deno::*;

pub fn init(&mut isolate: Isolate) -> Result<(), ErrBox> {
isolate.register_op("hello", op_hello); // register_op defined by #3002
isolate.execute("hello.js")?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be:

isolate.execute("hello.js", include_str!("hello.js"));`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably. I'm a bit concerned about double counting the source code if we do snapshots...

Ok(())
}

fn op_hello(_control_buf: &[u8], _zero_copy_buf: Option<PinnedBuf>) -> CoreOp {
println!("Hello world");
CoreOp::Sync(Box::new([]))
}

#[test]
fn js_test() {
isolate.execute("hello_test.js")
}

#[test]
fn rust_test() {
if let CoreOp::Sync(buf) = op_hello() {
assert_eq!(buf.len(), 0);
} else {
unreachable!();
}
}