extism / java-sdk

Extism Java Host SDK enables Java programs to embed and run WebAssembly plugins.
BSD 3-Clause "New" or "Revised" License
27 stars 7 forks source link

refactor: Update dependencies in java maven module and cleanup code #12

Closed thomasdarimont closed 7 months ago

thomasdarimont commented 1 year ago
thomasdarimont commented 1 year ago

@bhelx I gave this another round of refactoring:

refactor: Revise PluginTests and used API

refactor: Move Logging functionality to support package

refactor: Prevent instantiation of support classes

refactor: Improve developer experience

refactor: Add convenience methods to Extism for creating Manifests

refactor: Simplify usage of UserData with HostFunction

thomasdarimont commented 1 year ago

@bhelx I think the API could be improve quite a bit.

Naming of ExtismFunction / HostFunction

Currently the naming of ExtismFunction and HostFunction. It seems that "HostFunction" is in fact a wrapper/container for the actual host function which is then expressed as ExtismFunction.

How about changing the names in the following way: ExtismFunction -> HostFunctionCallback HostFunction -> HostFunction

HostFunction state and userdata

Another problem I see with HostFunction is that it (currently) transports a lot of baggage that's publicly accessible. In my refactoring I made the fields private and moved the callback as a local variable. However, the passed in userdata is still part of the HostFunction but that feels wrong to me. I think Userdata should be remove from the HostFunction and should rather be passed when calling the plugin, e.g. plugin.call(functionName, "someInput", myUserData);

Is user data meant to be custom metadata that could be added to a host function registration, or is this rather "per" call additional (typed) input?

If UserData should remain in HostFunction, I'd remove Optional parameter from the constructor and add 2 overloads, one with a UserData parameter and one without. This makes the call-sites simpler.

API quite low-level

In general the whole interface seems to be a bit too low-level, passing around pointers and LibExtism.ExtismValType's is quite cumbersome and error-prone. Perhaps a better abstraction could hide this internals. If users really have to work with the param and return types themselves, it might be cool to move the types LibExtism.ExtismValType, LibExtism.ExtismVal and LibExtism.ExtismValUnion as public static classes to Extism, e.g. Extism.ValType, Extism.Val, Extism.ValUnion.

Plugin lifecycle / reuse

Another thing that's still unclear to me is how users would initialize the plugin in their applications:

        try (var plugin = new Plugin(manifest, true, null)) {
            var output = plugin.call(functionName, "someInput");
           processOutput(output);
        }

Would they really create a new `Plugin instance per call, or would you rather create an instance per call? If the later is the case, then I think that the manifest will be parsed over and over again for each call, which can take some time. I think this could be improved a bit, since the wasm code will probably not change, in most applications, across plugin calls. So perhaps something like a PluginManager could (lazily) load / instanciate plugins once and (or on request) and use a "cached" and "prepared" plugin for the actual call. On explicit request or when the application shuts down the PluginManager could destroy / close() the created plugin instances. Are plugin instances intended to be shared or does every caller really need a dedicated plugin instance?

Logging integration

Currently, the deprecated org.extism.sdk.Extism#setLogFile is used to configure a log file through org.extism.sdk.LibExtism#extism_log_file. I think it doesn't look good to ship already deprecated functionality with a 1.0 release. How about providing a way to configure a logging callback in extism, which is called whenever a log line is generated in the library internally? With this in place, library integrations could register a logging callback as and adapt the log request to the logging facilities of the respective platform.

Remove unused components

Currently ManifestHttpRequest and org.extism.sdk.manifest.Manifest#allowedHosts are not used and could be removed. The functionality could be added later.

What do you think?

thomasdarimont commented 1 year ago

I also just tried the java host function example from the documentation, but it seems to be broken / outdated: https://extism.org/docs/integrate-into-your-codebase/java-host-sdk

The following host example would work (based on the current refactorings):

import org.extism.sdk.Extism;
import org.extism.sdk.ExtismFunction;
import org.extism.sdk.HostFunction;
import org.extism.sdk.HostUserData;
import org.extism.sdk.LibExtism.ExtismValType;
import org.extism.sdk.Plugin;

import java.nio.file.Path;

public class AppHostFuncs
{

  // we can create a custom type to pass complex, or compound, data 
  // through a host function. You could imagine this may hold a reference
  // to a database client or some other Java objects needed by the host functions
  public static class MyUserData extends HostUserData {

    private final String data1;

    private final int data2;

    public MyUserData(String data1, int data2) {
      this.data1 = data1;
      this.data2 = data2;
    }
  }

  // To create the host function we need a callback in Java world
  // and an ExtismFunction that references it
  public static HostFunction<?>[] getHostFunctions() {

    ExtismFunction<MyUserData> callbackFunc = (plugin, params, returns, data) -> {

      System.out.println("Hello from Java Host Function!");

      String wasmOutput = plugin.inputString(params[0]);
      System.out.printf("Input string received from plugin, %s%n", wasmOutput);

      data.ifPresent(d -> {
        System.out.printf("Host user data, %s, %d%n", d.data1, d.data2);

        // here you could deserialize the json into a java object again and process
        // with the help of the supplied user dataparams

        plugin.returnString(returns[0], "{ \"output\": " + wasmOutput + "}");
      });
    };

    var parametersTypes = new ExtismValType[]{ExtismValType.I64};
    var resultsTypes = new ExtismValType[]{ExtismValType.I64};

    var helloWorld = new HostFunction<>(
            "hello_world",
            parametersTypes,
            resultsTypes,
            callbackFunc,
            new MyUserData("test", 2)
    );

    return new HostFunction[]{helloWorld};
  }

  public static void main(String[] args) {

    var manifest = Extism.manifestFromPath(Path.of("code-functions.wasm"));
    var functions = getHostFunctions();

    // we must pass any host functions we created to the plugin constructor.
    // it will import these functions so the plugin can call them.
    try (var plugin = new Plugin(manifest, true, functions)) {
      var output = plugin.call("count_vowels", "Hello World");
      System.out.println(output);
    }
  }
}

The "simple" hello world looks like this with the refactorings:

import org.extism.sdk.Extism;
import org.extism.sdk.Plugin;

import java.nio.file.Path;

public class App 
{
    public static void main(String[] args)
    {
        var manifest = Extism.manifestFromPath(Path.of("code.wasm"));

        // NOTE: if you encounter an error such as: 
        // "Unable to load plugin: unknown import: wasi_snapshot_preview1::fd_write has not been defined"
        // change `false` to `true` in the following function to provide WASI imports to your plugin.
        try (var plugin = new Plugin(manifest, false)) 
        {
            var output = plugin.call("count_vowels", "Hello World");
            System.out.println(output);
        }
    }
}

The later almost becomes a one-liner:

import org.extism.sdk.Extism;
import org.extism.sdk.Plugin;

import java.nio.file.Path;

import static org.extism.sdk.Extism.manifestFromPath;

public class AppSlim {
    public static void main(String[] args) {
        var output = Extism.invokeFunction(manifestFromPath(Path.of("code.wasm")), "count_vowels", "Hello World");
        System.out.println(output);
    }
}
bhelx commented 1 year ago

This all looks great! I'm going to have to read it all and review on Monday, but overall I'm supportive of these changes.

thomasdarimont commented 7 months ago

Closing this outdated PR for now.