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

API changes to Ops #2730

Closed
3 of 4 tasks
ry opened this issue Aug 5, 2019 · 2 comments
Closed
3 of 4 tasks

API changes to Ops #2730

ry opened this issue Aug 5, 2019 · 2 comments

Comments

@ry
Copy link
Member

ry commented Aug 5, 2019

ops.rs has grown to over 2000 lines and needs to be broken up.

We additionally have seen performance problems for very hot ops like read/write with flatbuffers. This has necessitated the ability to handle non-flatbuffer message (see dispatch_minimal.ts). We want to allow ops to use their own serialization - flatbuffers, JSON, serde, or just a custom message format.

We want the ability to dynamically load (dlopen) ops that are not included in the deno executable. See preliminary work in #2385.

Thus I want to do the following changes:

  • refactor: split ops.rs #2753 Breaking up cli/ops.rs into cli/ops/read_write.rs, cli/ops/subprocess.rs, cli/ops/fetch.rs, ...

  • Create a new OpDispatcher trait which has a name in addition to a dispatch function. This allows us to create subtraits like FlatBufferOpDispatcher which would allow simplify a lot of the parsing/serialization code. Example code.

  • Add a new method to register op dispatchers dynamically. This would be called even for built-in ops at startup. isolate.register_op(op_read) feat: Use Isolate.register_op in CLI, take 2 #3039 (and others)

  • Add op_id throughout op API #2734 Add a new integer parameter to Deno.core.dispatch() to specify which op is being called.

let readId: number = null;
function read(fd: number, buf: Uint8Array): number {
  if (!readId) {
    readId = Deno.core.ops["read"]; // lazy lookup
  }
  /* ... */ 
  Deno.core.dispatch(readId, cntrolbuf, zerobuf);
  /* ... */
}

Note the new Deno.core.ops which allows us to retrieve an op id from a string. (We're worried that passing the string directly to Deno.core.dispatch would be slow)

@bartlomieju
Copy link
Member

  1. Breaking up cli/ops.rs into cli/ops/read_write.rs, cli/ops/subprocess.rs, cli/ops/fetch.rs, ...

I'll be working on this

ry added a commit to ry/deno that referenced this issue Aug 5, 2019
Deno.core.dispatch() used to push the "control" buf onto the shared
array buffer before calling into V8, with the idea that it was one less
argument to parse. Turns out there is no more overhead passing the
control ArrayBuffer directly over. Furthermore this optimization was
making the refactors outlined in denoland#2730 more complex. Therefore it is
being removed.
ry added a commit that referenced this issue Aug 6, 2019
Deno.core.dispatch() used to push the "control" buf onto the shared
array buffer before calling into V8, with the idea that it was one less
argument to parse. Turns out there is no more overhead passing the
control ArrayBuffer directly over. Furthermore this optimization was
making the refactors outlined in #2730 more complex. Therefore it is
being removed.
This was referenced Aug 6, 2019
@ry ry mentioned this issue Aug 14, 2019
43 tasks
ry pushed a commit that referenced this issue Aug 14, 2019
Note cli/dispatch_minimal.rs ops are not yet included in cli/ops.

This is part of work towards #2730
@ry
Copy link
Member Author

ry commented Oct 5, 2019

I consider this closed in #3039. I'm sure there are other op changes, but at least we can dynamically import ops now using Isolate::register_op

@ry ry closed this as completed Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants