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

afinch7 · 2019-08-30T21:27:24Z

depends on #2785
part of #2180
soft depends on switch to cargo for builds and snapshot gen refactors

Primary reasons for moving ops to standalone crates:

The complexity of cli can be divided into more manageable chunks.
Rebuilds should be faster in most cases, since we don't have to rebuild the code for every op always.
These new crates can be readily used in third party deno core implementations. This should remove a lot of the boilerplate for embedding.

Main goals in this PR:

Move fs ops to standalone crate that can be readily used in deno core implementations.
Provide some framework for other standalone ops in the future.
Avoid adding complexity to cli.
Test the versatility of my dispatcher system from New dispatcher infrastructure. #2785

TODOs:

Standardize permissions checks for standalone ops like this. This can be deferred for now, but still something to think on.
Make error kind system outwardly extensible. Maybe just use strings, since we are using json now?
Move TypeScript code for fs ops to this new standalone crate, and come up with a way to integrate this with the changes to snapshot generation(Refactor snapshot build #2825).

This also removes Deno.platform.

ry · 2019-08-31T01:28:57Z

Is it possible to get the typescript side of the ops in there too?

afinch7 · 2019-08-31T01:52:54Z

@ry I want to tackle that next. I figure it's best to wait for you to sort out #2825 first though.

hayd · 2019-09-03T17:36:04Z

I feel like a nicer API on the ts side would be:

class CopyFile extends Op {
    NAME = "copyFile"  // this is from the rust (used to lazily (?) lookup the opId)
}

export function copyFileSync(from: string, to: string): void {
    CopyFile.sendSync({ from, to });
}

export async function copyFile(from: string, to: string): Promise<void> {
    await CopyFile.sendAsync({ from, to });
}


// or perhaps, rather than a subclass
const copyFile = new Op("copyFile");

// or perhaps even
const { sendSync, sendAsync } = op("copyFile");

hayd

There's a lot of churn in this diff, which makes reviewing it challenging.
Anyway, I read through and added some comments.

One question: I don't understand why op registering is required at the ts level?
Once an op is registered in rust (namespace, opName) why isn't looking up the opId sufficient to send?

hayd · 2019-09-05T03:57:25Z

cli/ops/compiler.rs

 use crate::state::ThreadSafeState;
 use crate::tokio_util;
 use deno::*;

+// Cache


These comments are unnecessary.

👍 They seemed helpful before I reduced the amount of code for each op.

cli/ops/mod.rs

hayd · 2019-09-05T04:04:31Z

cli_snapshots/build.rs

+  let import_maps = vec![
+    deno_ops_fs_bundle::get_import_map(),
+    deno_bundle_util::get_import_map(),
+    deno_dispatch_json_bundle::get_import_map(),


I don't really understand the purpose/necessity of this import map stuff.

It should't be necessary for deno cli, but I'm trying to keep deno core embedding in mind here.
No matter where the source for deno_ops_fs_bundle is in relation to the project it's being used in, if I include the import maps deno_ops_fs will always resolve to the absolute path to the library entry point.

hayd · 2019-09-05T04:12:45Z

dispatch_json/bundle/main.ts

+  ) {
+    ops[this.opNamespace][this.opName] = (id?: number): void => {
+      this.opId = id;
+      if (id !== undefined) {


This should throw if id === undefined (not sure if that case is possible).

undefined is used to signal a "reset" to listeners, since I can't really unsetAsyncHandler currently we just ignore. The only time a "reset" happens is when you call Isolate::set_dispatcher_registry(only when creating a new worker in our case). setAsyncHandler has no way to "correctly" handle a opId value that isn't a number, thus passing undefined will throw a error.

hayd · 2019-09-05T04:14:18Z

js/dispatch.ts

+export let OP_START: OpId;
+opNamespace.start = (id: MaybeOpId): void => {
+  OP_START = id!;
+  core.setAsyncHandler(id!, json.asyncMsgFromRust);


This API is strange...

I'm using a class based API like the one you suggested in all new implementations. Consider this depreciated.

hayd · 2019-09-05T04:17:05Z

ops/fs/bundle/chown.ts

+import { JsonOp } from "deno_dispatch_json";
+import { opNamespace } from "./namespace.ts";
+
+const OP_CHOWN = new JsonOp(opNamespace, "chown");


The opNamespace (which is always "builtin" seems uncessary), maybe export a function that wraps this:

const OP_CHOWN = builtinOp("chown") // which also allows (if one op in the file) const { sendSync, sendAsync } = builtinOp("chown")

I'm going to make opNamespace configurable. If I wanted to include multiple versions/instances of the same ops in a project I should be able to.

afinch7 · 2019-09-09T16:19:39Z

One question: I don't understand why op registering is required at the ts level?

It's not you can just as easily use sendSync(Deno.ops.namespace.opName, data), but this will call a get accessor every time(slower). A lot of this is explained in the prerequisite PR(#2785).

hayd · 2019-09-09T17:15:54Z

But couldn't that lookup be moved into the class constructor? Then it's only done once and avoids the accessor. On the ts side it's more "lookup" than "register"? Perhaps I'm just not following, I'll reread.

This is an exciting change, looking forward to it.

afinch7 · 2019-09-09T18:16:04Z

On the ts side it's more "lookup" than "register"? Perhaps I'm just not following, I'll reread.

Yes, sort of.
Assignments assign a listener:

// This function will get called when rust side registers a
// op named "opName" in namespace "namespace", or
// immediately if the op has already been registered at 
// the time of assignment.
let someValue;
Deno.ops.namespace.opName =  (id) => { someValue = id };

Read/get performs a direct lookup(returns whatever value is available at the time):

// Gets current value. May be undefined.
const someValue = Deno.ops.namespace.opName;

The api uses a fancy set accessor to register "listener" functions that will be informed as soon as a op is registered on the rust side. https://github.com/afinch7/deno/blob/dbe541f37baa9d910989402ef5fb815e257fdbff/core/core_lib.js#L242-L274

Basically

Deno.ops.namespace.opName = (id) => { someValue = id };

is equivalent to something like this

Deno.on("registerOp", (e) => {
  if (e.namespace === "namespace" && e.name = "opName") {
    someValue = e.id;
  }
});

This seems unnecessary, but accessors are slow and we don't want to expose things directly to avoid tampering.

But couldn't that lookup be moved into the class constructor? Then it's only done once and avoids the accessor.

The op ids we need only exist at runtime, so preforming a direct lookup during snapshot creation would just give us a inaccurate or undefined value for each op id.

ry · 2019-09-09T18:33:02Z

I agree with the premiss of this PR but +3000 LoC is unreasonably large when there is no new functionality being added... We need to find a way to do this in smaller pieces and less verbosely. If anything this PR should be removing code, not adding more.

afinch7 · 2019-09-09T18:43:48Z

@ry 2/3 of the added lines of code are from #2785, but I still want to try to clean this up a lot.

ry · 2019-10-02T17:18:45Z

This PR is out of date and can't be landed so I'm closing.

It's part of an ongoing discussion about how to organize "op crates". See also #3025 and #3031.

afinch7 force-pushed the move_fs_to_crate branch from 511d14d to 169ac02 Compare August 30, 2019 22:09

afinch7 force-pushed the move_fs_to_crate branch 2 times, most recently from cba256c to 397174f Compare September 3, 2019 16:04

hayd reviewed Sep 5, 2019

View reviewed changes

Refactor dispatcher system

dbe541f

afinch7 force-pushed the move_fs_to_crate branch from 4b3f6d9 to 2c6d6c6 Compare September 9, 2019 16:16

afinch7 marked this pull request as ready for review September 9, 2019 16:19

move fs ops to standalone crate

9ee33c7

afinch7 force-pushed the move_fs_to_crate branch from 2c6d6c6 to 9ee33c7 Compare September 9, 2019 18:16

bartlomieju mentioned this pull request Sep 19, 2019

Dispatch and native plugins #2987

Closed

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] Move fs ops to standalone crate. #2840

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

afinch7 commented Aug 30, 2019 •

edited

Loading

ry commented Aug 31, 2019

afinch7 commented Aug 31, 2019 •

edited

Loading

hayd commented Sep 3, 2019 •

edited

Loading

hayd left a comment

hayd Sep 5, 2019

afinch7 Sep 5, 2019

hayd Sep 5, 2019

afinch7 Sep 5, 2019 •

edited

Loading

hayd Sep 5, 2019

afinch7 Sep 5, 2019

hayd Sep 5, 2019

afinch7 Sep 5, 2019

hayd Sep 5, 2019

afinch7 Sep 5, 2019

afinch7 commented Sep 9, 2019

hayd commented Sep 9, 2019

afinch7 commented Sep 9, 2019

ry commented Sep 9, 2019

afinch7 commented Sep 9, 2019

ry commented Oct 2, 2019

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

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

Conversation

afinch7 commented Aug 30, 2019 • edited Loading

ry commented Aug 31, 2019

afinch7 commented Aug 31, 2019 • edited Loading

hayd commented Sep 3, 2019 • edited Loading

hayd 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

afinch7 Sep 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afinch7 commented Sep 9, 2019

hayd commented Sep 9, 2019

afinch7 commented Sep 9, 2019

ry commented Sep 9, 2019

afinch7 commented Sep 9, 2019

ry commented Oct 2, 2019

afinch7 commented Aug 30, 2019 •

edited

Loading

afinch7 commented Aug 31, 2019 •

edited

Loading

hayd commented Sep 3, 2019 •

edited

Loading

afinch7 Sep 5, 2019 •

edited

Loading