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

Feature: Native plugins #2385

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@afinch7
Copy link
Contributor

commented May 21, 2019

See denoland/deno_std#475 for WIP example usage.

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch from 0e7f8f2 to 3dc6645 May 21, 2019

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

I might need some help with adding the cargo and libloading crates to third_party still having a few build errors with some dependencies.

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

It's about 90% there at this point just need to fix some bugs, and figure out what to do with the new dependencies I added. Major TODOs still left:

  • Solve dependency build issues or find a way to remove some dependencies
  • Fix compiler getting stuck on module name I.E. Cargo.toml:11:30 - error TS8010: 'types' can only be used in a .ts file.
  • Implement dispatch for custom ops
  • Fill BindingDispatchContext with useful context functions and implement them
  • Add a bunch of new tests for this
  • Add permissions for loading native bindings

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch from 7cf0f56 to 04fc630 May 26, 2019

@afinch7 afinch7 referenced this pull request May 26, 2019

Open

Add rust crate 'dlopen' #44

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch 2 times, most recently from 1619c3e to e8ad99b May 26, 2019

@afinch7 afinch7 marked this pull request as ready for review May 26, 2019

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch from dc4705c to bc0a410 May 26, 2019

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Mostly finished at this point. Ended up dropping cargo integration, since it adds so much complexity. I might revisit cargo integration later. I also haven't added support for native binding loading via url.
The current usage is just to import the compiled dylib:

import { someOp } from "./binding_project/target/release/libbinding_project.so";

This poses some problems for cross platform mainly that the built artifact name varies from platform to platform. I didn't want to introduce any magic I.E. ./binding_project/target/release/binding_project -> ./binding_project/target/release/libbinding_project.so on linux.
I considered maybe a special extension to make it more obvious where this magic comes into play I.E. ./binding_project/target/release/binding_project.binding, but that also seems a little too magic as well.
What do you think @ry?

I also might need some help figuring out the current issue on appveyor or finding a simpler way to build my test binding plugin. I'm currently waiting on my changes to dlopen_derive to go through, so I don't have to setup builds for multiple versions of quote and syn.

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch 2 times, most recently from f6fe31e to d30c877 May 27, 2019

@bartlomieju bartlomieju referenced this pull request May 29, 2019

Open

deno-gyp #1654

@ry

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

This is an awesome patch. Amazing work.

Adding a native bindings API needs to be considered very carefully. It has impacts on security (all bets are off when you execute some random native code), performance, and usability (the GYP situation in Node is one of my largest blunders).

I am not ready to commit to importing .so files via the module system. For one thing, this breaks browser compatibility; not such a big deal itself. It introduces need for declare_binding_plugin!. I wouldn't rule this out in the future, but I think for this first pass we shouldn't try to integrate dlopen into the existing module system.

There's also the question of how these .so files be built. It would be great to be able to import a Cargo.toml (as you had earlier) but this means we depend on some external tooling. (Depending on cargo is probably fine, but it needs to be considered as it would break the promise of Deno being completely dependency free.) I'd rather punt on the topic of integrating with a build system.

I propose having a dlopen op, and another op to call a function in the DLL (dlcall?). I think it's fine if we just support calling functions that have a specific function signature. I'd leave it up to you to how you think the most minimal function signature is. Something like typedef void verysimple(size_t len, void* data); would work for this first PR - but maybe that's too simple to get the example working. I think another good choice would be DispatchFn in core/isolate.rs:

type DispatchFn = Fn(&[u8], Option<PinnedBuf>) -> Op;

I think making these changes will reduce the surface area of this proposal and allow us to land something basic before committing to defining how everything works upfront.

I suggest renaming --allow-native-bindings to --allow-dlopen.

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Great feedback. I needed to get out of my one track mindset a little here. Since I already decided that direct cargo import is a little too complicated for this one, I don't really see any reason it has to be a dylib that loads typescript instead of the other way around.
Thoughts on this TypeScript api?

export interface DynamicLibFn {
  dispatchSync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Uint8Array;

  dispatchAsync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Promise<Uint8Array>;
}

// A loaded dynamic lib function.
// Loaded functions will need to loaded and addressed by unique identifiers
// for performance, since loading a function from a library for every call 
// would likely be the limiting factor for many use cases.
/** @internal */
class DynamicLibFnImpl implements DynamicLibFn {
  
  private readonly dlFnId: number;

  constructor(dlId: number, name: string) {
    this.dlFnId = loadDlFn(dlId, name);
  }

  dispatchSync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Uint8Array {
    // Like the prior Deno.nativeBindings.sendSync
    return callDlFnSync(this.dlFnId, data, zeroCopy);
  }

  async dispatchAsync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Promise<Uint8Array> {
    // Like the prior Deno.nativeBindings.sendSync but async
    return callDlFnAsync(this.dlFnId, data, zeroCopy);
  }

}

export interface DynamicLib {
  loadFn(name: string): DynamicLibFn;
}

// A loaded dynamic lib.
// Dynamic libraries need to remain loaded into memory on the rust side
// ,and then be addressed by their unique identifier to avoid loading
// the same library multiple times.
export class DynamicLibImpl implements DynamicLib {

  // unique resource identifier for the loaded dynamic lib rust side
  private readonly dlId: number;
  private readonly fnMap: Map<string, DynamicLibFn> = new Map();

  constructor(private readonly libraryPath: string) {
    // call some op to load the library and get a resource identifier
    this.dlId = loadDl(libraryPath);
  }

  loadFn(name: string): DynamicLibFn {
    // call some op to load the fn from this library and get resource identifier
    const cachedFn = this.fnMap.get(name);
    if (cachedFn) {
      return cachedFn;
    } else {
      const dlFn = new DynamicLibFnImpl(this.dlId, name);
      this.fnMap.set(name, dlFn);
      return dlFn;
    }
  }

}

The implementation for the test plugin/binding being something like this

const { platformFilename, DynamicLibary } = Deno.dlopen;

const dLib = new DynamicLibary("target/release/" + platformFilename("test_binding_lib"));
const testOpFn = dLib.loadFn("test_op");

export interface TestOptions {
    name: string;
}

export interface TestResponse {
    data: Uint8Array;
}

const textEncoder = new TextEncoder();

function encodeTestOp(args: TestOptions): Uint8Array {
    return textEncoder.encode(JSON.stringify(args));
}

const textDecoder = new TextDecoder();

function decodeTestOp(data: Uint8Array): any {
    return textDecoder.decode(data);
}

export const testOp = (args: TestOptions): any => {
    return decodeTestOp(
        testOpFn.dispatchSync(
            encodeTestOp(args),
        ),
    );
}
@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@ry I made some major simplifications, and removed the import functionality PTAL.

Show resolved Hide resolved cli/msg.fbs Outdated
Show resolved Hide resolved cli/msg.fbs Outdated
Show resolved Hide resolved cli/msg.fbs
Show resolved Hide resolved cli/msg.fbs
Show resolved Hide resolved cli/ops.rs Outdated
Show resolved Hide resolved cli/ops.rs Outdated
Show resolved Hide resolved cli/ops.rs Outdated
Show resolved Hide resolved js/deno.ts Outdated
Show resolved Hide resolved js/deno.ts Outdated
Show resolved Hide resolved js/dlopen.ts Outdated
Show resolved Hide resolved js/dlopen.ts Outdated
Show resolved Hide resolved js/dlopen.ts Outdated
Show resolved Hide resolved js/dlopen.ts Outdated
Show resolved Hide resolved js/dlopen.ts Outdated

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch 4 times, most recently from ba7e7d1 to 0f67f8d Jun 18, 2019

Show resolved Hide resolved cli/flags.rs Outdated
Show resolved Hide resolved test_plugin/Cargo.toml Outdated
Show resolved Hide resolved test_plugin/lib.rs Outdated
Show resolved Hide resolved core/plugins.rs Outdated
Show resolved Hide resolved test_plugin/main.ts Outdated

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch 2 times, most recently from 16ea973 to 1003661 Jun 25, 2019

Show resolved Hide resolved tests/034_native_plugins.ts Outdated
Show resolved Hide resolved tests/034_native_plugins.ts Outdated
Show resolved Hide resolved .appveyor.yml Outdated
.travis.yml Outdated
@@ -73,7 +72,8 @@ script:
- ./tools/lint.py
- ./tools/test_format.py
- ./tools/build.py -C target/release
- DENO_BUILD_MODE=release ./tools/test.py
- cargo build -p test_plugin --release --locked

This comment has been minimized.

Copy link
@ry

ry Jun 25, 2019

Collaborator

I don't like that this breaks my usual ./tools/build.py && ./tools/test.py

Let me see if I can whip up a BUILD.gn to do this...

This comment has been minimized.

Copy link
@ry

ry Jun 25, 2019

Collaborator

I've added the BUILD.gn file... it's a bit hacky but ./tools/build.py && ./tools/test.py -p 034 works.

@@ -1,7 +1,7 @@
const { openPlugin, pluginFilename, env } = Deno;

const plugin = openPlugin(
env().DENO_BUILD_PATH + "/" + pluginFilename("test_plugin")
env().DENO_BUILD_PATH + "/rust_crates/" + pluginFilename("test_plugin")

This comment has been minimized.

Copy link
@ry

ry Jun 25, 2019

Collaborator

This is unfortunate...

This comment has been minimized.

Copy link
@afinch7

afinch7 Jun 26, 2019

Author Contributor

It is, but it works. Any idea what is up with the appveyor build?

This comment has been minimized.

Copy link
@afinch7

afinch7 Jun 27, 2019

Author Contributor

tools/build.py --release works fine on windows, but tools/build.py doesn't for some reason.

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch from 15f53d9 to 49817d0 Jun 26, 2019

@afinch7 afinch7 referenced this pull request Jun 28, 2019

Merged

Update rust for ci #2599

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch from 49817d0 to 2ac483e Jun 28, 2019

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

This patch resolves the issue with windows, but it wouldn't really be a debug build anymore

diff --git a/config/BUILD.gn b/config/BUILD.gn
index 3f37c75c1..d41dd38ef 100644
--- a/config/BUILD.gn
+++ b/config/BUILD.gn
@@ -148,7 +148,6 @@ config("feature_flags") {
 
 config("debug") {
   defines = [
-    "_DEBUG",
     "DYNAMIC_ANNOTATIONS_ENABLED=1",
     "WTF_USE_DYNAMIC_ANNOTATIONS=1",
   ]
diff --git a/config/win/BUILD.gn b/config/win/BUILD.gn
index b5a58459a..9963db297 100644
--- a/config/win/BUILD.gn
+++ b/config/win/BUILD.gn
@@ -422,7 +422,7 @@ config("release_crt") {
 config("dynamic_crt") {
   if (is_debug) {
     # This pulls in the DLL debug CRT and defines _DEBUG
-    cflags = [ "/MDd" ]
+    cflags = [ "/MD" ]
   } else {
     cflags = [ "/MD" ]
   }
@@ -431,7 +431,7 @@ config("dynamic_crt") {
 config("static_crt") {
   if (is_debug) {
     # This pulls in the static debug CRT and defines _DEBUG
-    cflags = [ "/MTd" ]
+    cflags = [ "/MT" ]
   } else {
     cflags = [ "/MT" ]
   }

It seems like we need to add a arg to the linker via -Clink-args ideally near this

args += [
"-Clinker-flavor=lld-link",
"-Clinker=" + rebase_path(
"//third_party/llvm-build/Release+Asserts/bin/lld-link.exe",
root_build_dir),
]
I.E.

if (is_debug) {
  args += "-Clink-args='/someflag'"
}

@ry ry referenced this pull request Jul 2, 2019

Open

cargo should be the main build frontend #2608

1 of 6 tasks complete

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch 2 times, most recently from 1961a12 to 4d3862f Jul 2, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@afinch7 You simply skip the plugin build & tests when is_win && is_debug ? I forgot that we don't even test that in the 'cargo build' we always force release

deno/tools/gn.rs

Lines 20 to 23 in 32cde32

// On Windows, we need to link with a release build of libdeno, because
// rust always uses the release CRT.
// TODO(piscisaureus): make linking with debug libdeno possible.
String::from("release")

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

In further testing I discovered that asyncs don't work correctly. futures::task::current() is completely broken in any loaded plugins. This is a limitation of the current version of the futures crate. The only real solution here is to convert everything to use the new futures api that landed with rust 1.36.0.

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@ry I don't think so. Also I forgot to mention that patch is for //build. Maybe just add "cargo build -p test_plugin ${release ? '--release' : '' }" or something similar to //tools/build.py for now? Your plan is to move all builds to cargo in the future right?

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

Maybe just add "cargo build -p test_plugin ${release ? '--release' : '' }" or something similar to //tools/build.py for now?

That works

@afinch7 afinch7 force-pushed the afinch7:native_bindings branch from 80fe8f1 to fb33832 Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.