op_dispatcher.rs

use std::collections::HashMap;
use std::sync::Mutex;

#[macro_use]
extern crate lazy_static;

struct CoreOp {} // Placeholder.
struct FlatBuffer {} // Placeholder.
struct PinnedBuf {} // Placeholder.

impl FlatBuffer {
    fn parse(_blob: &[u8]) -> Self {
        unimplemented!()
    }
}

// Use a separate trait to give the op a name, because a trait object
// object (dyn Opdispatcher) can't have associated constants.
trait NamedOpDispatcher: OpDispatcher {
    const NAME: &'static str;
}

trait OpDispatcher: Send + Sync {
    fn dispatch(&self, args: &[u8], buf: Option<PinnedBuf>) -> CoreOp;
}

trait FlatBufferOpDispatcher: Send + Sync {
    fn dispatch_flatbuffer(&self, fb: &FlatBuffer, buf: Option<PinnedBuf>) -> CoreOp;
}

impl<T> OpDispatcher for T
where
    T: FlatBufferOpDispatcher,
{
    fn dispatch(&self, args: &[u8], buf: Option<PinnedBuf>) -> CoreOp {
        let fb = FlatBuffer::parse(args); // Or something like that.
        return self.dispatch_flatbuffer(&fb, buf);
    }
}

fn register_op<D: NamedOpDispatcher + 'static>(d: D) {
    // Rather than having a global registry, I think it should be per-isolate.
    lazy_static! {
        static ref OP_REGISTRY: Mutex<HashMap<&'static str, Box<dyn OpDispatcher>>> =
            Mutex::new(HashMap::new());
    }
    OP_REGISTRY
        .lock()
        .unwrap()
        .entry(D::NAME)
        .and_modify(|_| panic!("Op already registered"))
        .or_insert(Box::new(d));
}

// Example implements OpDispatcher.
struct BasicOpDispatcherExample;
impl NamedOpDispatcher for BasicOpDispatcherExample {
    const NAME: &'static str = "Basic";
}
impl OpDispatcher for BasicOpDispatcherExample {
    fn dispatch(&self, _args: &[u8], _buf: Option<PinnedBuf>) -> CoreOp {
        unimplemented!()
    }
}

// Example implements FlatBufferOpDispatcher.
struct SomeFlatBufferOpDispatcher;
impl NamedOpDispatcher for SomeFlatBufferOpDispatcher {
    const NAME: &'static str = "FB";
}
impl FlatBufferOpDispatcher for SomeFlatBufferOpDispatcher {
    fn dispatch_flatbuffer(&self, _fb: &FlatBuffer, _buf: Option<PinnedBuf>) -> CoreOp {
        unimplemented!()
    }
}

fn main() {
    register_op(BasicOpDispatcherExample);
    register_op(SomeFlatBufferOpDispatcher);
    register_op(SomeFlatBufferOpDispatcher); // Crash.
}

// Rather than having a global registry, I think it should be per-isolate.

Agreed, that would solve issue with access to special ops for TS compiler isolate (op_fetch_module_meta_data, op_cache).

I'm not sure about register_op - you suggested to pass NamedOpDispatcher as an argument and registers a single op. This seems cumbersome, my idea was that you'd register concrete instances of dispatchers that in turn have mapping of op -> handler. During registration of dispatcher it's verified that this dispatcher wasn't registered before, as well as verified there are no duplicate op names.

trait OpDispatcher: Send + Sync {
    lazy_static! {
        static ref OP_REGISTRY: Mutex<HashMap<&'static str, Box<dyn Op>>> =
            Mutex::new(HashMap::new());
    }
   
   fn register_ops(&self) {};

   fn register_op(&self, name: &str, handler: dyn Op) {
       OP_REGISTRY
        .lock()
        .unwrap()
        .entry(name)
        .and_modify(|_| panic!("Op already registered"))
        .or_insert(handler);
    }

    fn dispatch(&self, args: &[u8], buf: Option<PinnedBuf>) -> CoreOp;
}

trait NamedOpDispatcher: OpDispatcher {
    const NAME: &'static str;
}

impl NamedOpDispatcher for CliOpDispatcher {
    const NAME: &'static str = "cli";

    ... everything we have in `/cli/ops.rs` now....
}

impl NamedOpDispatcher for TsCompilerOpDispatcher {
    const NAME: &'static str = "ts";
   
    fn register_ops(&self) {
      self.register_op("fetch_module_meta_data", self.op_fetch_module_meta_data);
      self.register_op("cache", self.op_cache);
    }

    fn op_fetch_module_meta_data() -> CoreOp;
    fn op_cache() -> CoreOp;
}

fn register_dispatcher<D: NamedOpDispatcher + 'static>(d: D) {
    lazy_static! {
        static ref DISPATCHER_REGISTRY: Mutex<HashMap<&'static str, Box<dyn OpDispatcher>>> =
            Mutex::new(HashMap::new());
        static ref OP_REGISTRY: Mutex<HashMap<&'static str, Box<dyn Op>>> =
            Mutex::new(HashMap::new());
    }

   DISPATCHER_REGISTRY
        .lock()
        .unwrap()
        .entry(D.name)
        .and_modify(|_| panic!("Dispatcher already registered"))
        .or_insert(Box::new(D));

    for op_name, handler in D.OP_REGISTRY.iter() {
       OP_REGISTRY
        .lock()
        .unwrap()
        .entry(op_name)
        .and_modify(|_| panic!("Op already registered"))
        .or_insert(handler);
    }
}

fn main() {
   let cli_dispatch = CliOpDispatcher{};
   register_dispatcher(cli_dispatch);
   let ts_dispatch = TsCompilerOpDispatcher{};
   register_dispatcher(ts_dispatch);
}

So there's two layers of registries, but that shouldn't really hurt performance because registration will happen only once on startup. Advantage: ops can be nicely grouped by domain in a single dispatcher.

Then during Deno startup we can send full mapping with ops

So instead of this:

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);
  /* ... */
}

We'd get this:

// during startup
Deno.core.registerOps = function(opMapping: Record<string, number>) {
   Deno.core.ops = opMapping;
   Object.freeze(Deno.core.ops);
}

// files.ts
function read(fd: number, buf: Uint8Array): number {
  /* ... */ 
  Deno.core.dispatch(Deno.core.ops.read, controlbuf, zerobuf);
  /* ... */
}

WDYT? CC @piscisaureus @ry

my idea was that you'd register concrete instances of dispatchers

I think that's reasonable.

Object.freeze(Deno.core.ops);

I think that would introduce problems for dlopen.

Object.freeze(Deno.core.ops);

I think that would introduce problems for dlopen.

You're right. Then probably some getter/setter structure would make sense. The idea is to prevent users from meddling with Deno.core.ops, but yeah, ability to register additional ops dynamically is required.

Namespacing maybe?

type OpsObject =  { [namespace: string]: { [name: string]: number } };

class Ops {
  
  private readonly records: OpsObject = {};
  // Cloned version of records that is frozen, so we can avoid cloning records for every access.
  private recordsFrozen: OpsObject = Object.freeze(Object.assign({}, this.records));

  constructor() {}

  get ids(): OpsObject {
    return this.recordsFrozen;
  }
  
  add(namespace: string, name: string, id: number) {
    if (this.records[namespace] === undefined) {
        this.records[namespace] = {};
    }
    this.records[namespace][name] = id;
    // Clone, freeze, and assign records to recordsFrozen.
    this.recordsFrozen = Object.freeze(Object.assign({}, this.records));
  }
}

Deno.core.ops = new Ops();

Object.seal(Deno.core.ops);

// during startup
Deno.core.registerOp = Deno.core.ops.add;

// files.ts
function read(fd: number, buf: Uint8Array): number {
  /* ... */ 
  Deno.core.dispatch(Deno.core.ops.ids.builtin.read, controlbuf, zerobuf);
  /* ... */
}

New dlopen/plugins code would be something like this:

export type PluginCallReturn = Uint8Array | undefined;

export interface PluginOp {
  dispatch(
    data: Uint8Array,
    zeroCopy?: ArrayBufferView
  ): Promise<PluginCallReturn> | PluginCallReturn;
}

class PluginOpImpl implements PluginOp {

   constructor(
     private readonly pluginRid: number,
     private readonly name: string,
  ) {
    // assert op id is present
    assert(Deno.core.ops.ids["plugin_" + this.pluginRid][name] !== undefined);
  }

   dispatch(
    data: Uint8Array,
    zeroCopy?: ArrayBufferView
  ): Promise<PluginCallReturn> | PluginCallReturn {
    const response = Deno.core.dispatch(Deno.core.ops.ids["plugin_" + this.pluginRid][name], data, zeroCopy);
    // handle response(create promise if async)
  }
}

export interface Plugin {
  loadOp(name: string): PluginOp;
}

export class PluginImpl implements Plugin {
  // unique resource identifier for the loaded dynamic lib rust side
  private readonly rid: number;
  private readonly opSet: Set<string> = new Set();

  constructor(libraryPath: string) {
    this.rid = dlOpen(libraryPath);
  }

   loadOp(name: string): PluginOp {
    if (!this.opSet.has(name)) {
      // Ensure op is registed and Deno.core.registerOp gets called with the op id.
      loadPluginOp(this.rid, name);
      this.opSet.add(name);
    }
    let op = new PluginOpImpl(this.rid, name);
    return op;
  }
}