WIP a prototype crate design for ops #3025

ry · 2019-09-26T04:26:19Z

browse files at https://github.com/ry/deno/tree/op_hello/op_hello

super minimal MVP would be pure JS, https://github.com/ry/deno/tree/op_hello/op_hello_js

This isn't working, just trying to imagine what the ideal structure would look like.

ry · 2019-09-26T04:43:02Z

op_hello/src/hello_test.ts

+// 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";


@afinch7 is this along the lines of what you were thinking?

This looks good. It's very nice that all the relevant info is encoded into standard elements of the url structure. No need to design fancy parsing.

bartlomieju

I like the ideas outlined in this PR - they do not require too much magic, but we still need to figure out ops registration in JS

bartlomieju · 2019-09-26T09:10:19Z

op_hello/src/hello_test.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";


// 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 👍

bartlomieju · 2019-09-26T09:12:31Z

op_hello/src/hello.ts

@@ -0,0 +1,12 @@
+const OP_HELLO: number = Deno.ops.add("hello");


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

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?

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

@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

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...

Should I update #3002 to use this API?

If you're in agreement, yea

@ry PR updated

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.

bartlomieju · 2019-09-26T10:04:40Z

@ry could we game out example with "json" dispatch in this PR as well? It's gonna be most often used method to write ops so it'd be nice to see how we can fit it in this model

ry · 2019-09-26T10:09:29Z

@bartlomieju I'd rather land an example crate that uses core only first. I think it keeps us honest about the interfaces. There are bigger problems of how to get ThreadSafeState into the ops - but it can be solved maybe by introducing a special type of init function which receives a deno_cli::Worker instead of deno::Isolate. We want to still enable some typescript hater to build a "deno.js" style project. So let's make this work in the simple case first in core, then later build on that structure to deliver cli op crates.

That is to say, I'm not sure how exactly json dispatch in this crate format should look, but I'm sure we can find some trick to make it not terrible : )

bartlomieju · 2019-09-26T10:14:11Z

There are bigger problems of how to get ThreadSafeState into the ops - but it can be solved maybe by introducing a special type of init function which receives a deno_cli::Worker instead of deno::Isolate.

I figured it out in #3004, please see cli/worker.rs. That solution can be generalized to provide any additional arguments to ops as long as trait boundaries are satisfied.

We want to still enable some typescript hater to build a "deno.js" style project. So let's make this work in the simple case first in core, then later build on that structure to deliver cli op crates.

That's fine for me, I can tinker on my own

bartlomieju · 2019-09-27T11:01:49Z

op_hello_js/lib.rs

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


I think it should be:

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

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

bartlomieju · 2019-09-27T11:16:53Z

op_hello/src/lib.rs

+  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")


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

Yes we should support that too.

ry · 2019-10-02T17:20:32Z

Closing because this isn't intended to be landed. Just an example branch.

ry added 4 commits September 26, 2019 00:19

prototype op_hello

282b175

WIP

404892c

wip

2c203d7

wip

ce4ae67

ry commented Sep 26, 2019

View reviewed changes

ry added 6 commits September 26, 2019 01:52

x

834a2b9

x

775f56a

Maybe we can use split file namespaces

e9e0e80

elegant url

21aa00a

x

d8c20c0

Add op_hello_js

607aeae

bartlomieju reviewed Sep 26, 2019

View reviewed changes

ry added 2 commits September 26, 2019 05:44

simplify

f6f169d

adjust

b04616a

bartlomieju reviewed Sep 27, 2019

View reviewed changes

This was referenced Sep 27, 2019

example: prototype op crate #3031

Closed

feat: Use Isolate.register_op in CLI, take 2 #3039

Merged

ry mentioned this pull request Oct 2, 2019

[WIP] Move fs ops to standalone crate. #2840

Closed

3 tasks

ry closed this Oct 2, 2019

ry mentioned this pull request Oct 29, 2019

Using Rust as Backend? #3240

Closed

rsp mentioned this pull request Nov 17, 2019

Need help porting uWS (C++) for fast HTTP/WebSocket server #3366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP a prototype crate design for ops #3025

WIP a prototype crate design for ops #3025

ry commented Sep 26, 2019 •

edited

Loading

ry Sep 26, 2019

afinch7 Sep 29, 2019

bartlomieju left a comment

bartlomieju Sep 26, 2019

bartlomieju Sep 26, 2019

bartlomieju Sep 26, 2019 •

edited

Loading

ry Sep 26, 2019

bartlomieju Sep 26, 2019

ry Sep 26, 2019 •

edited

Loading

bartlomieju Sep 26, 2019

ry Sep 26, 2019

bartlomieju Sep 26, 2019

afinch7 Sep 29, 2019

bartlomieju commented Sep 26, 2019

ry commented Sep 26, 2019 •

edited

Loading

bartlomieju commented Sep 26, 2019

bartlomieju Sep 27, 2019

ry Sep 27, 2019

bartlomieju Sep 27, 2019

ry Sep 27, 2019

ry commented Oct 2, 2019

		@@ -0,0 +1,12 @@
		const OP_HELLO: number = Deno.ops.add("hello");

WIP a prototype crate design for ops #3025

WIP a prototype crate design for ops #3025

Conversation

ry commented Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Sep 26, 2019 • edited Loading

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

bartlomieju commented Sep 26, 2019

ry commented Sep 26, 2019 • edited Loading

bartlomieju commented Sep 26, 2019

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

ry commented Oct 2, 2019

ry commented Sep 26, 2019 •

edited

Loading

bartlomieju Sep 26, 2019 •

edited

Loading

ry Sep 26, 2019 •

edited

Loading

ry commented Sep 26, 2019 •

edited

Loading