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.

Sign up for GitHub

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

Jump to bottom

RFC - Human friendly HostImports' builders #445

Closed
wants to merge 1 commit into from
Closed

RFC - Human friendly HostImports' builders #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
256 changes: 256 additions & 0 deletions runtime/src/main/java/com/dylibso/chicory/runtime/HostImports.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,21 @@
package com.dylibso.chicory.runtime;

import com.dylibso.chicory.wasm.types.Limits;
import com.dylibso.chicory.wasm.types.MemoryLimits;
import com.dylibso.chicory.wasm.types.MutabilityType;
import com.dylibso.chicory.wasm.types.Table;
import com.dylibso.chicory.wasm.types.Value;
import com.dylibso.chicory.wasm.types.ValueType;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.function.Function;
import java.util.function.IntConsumer;
import java.util.function.IntSupplier;
import java.util.function.IntUnaryOperator;
import java.util.function.LongConsumer;
import java.util.function.LongSupplier;
import java.util.function.LongUnaryOperator;

public class HostImports {
private static final HostFunction[] NO_HOST_FUNCTIONS = new HostFunction[0];
Expand Down Expand Up @@ -137,6 +150,10 @@ public static Builder builder() {
return new Builder();
}

public static Builder builder(HostImports imports) {
return new Builder(imports);
}

public static final class Builder {
private List<HostFunction> functions;
private List<HostGlobal> globals;
Expand All @@ -146,6 +163,245 @@ public static final class Builder {

Builder() {}

Builder(HostImports imports) {
this.functions = new ArrayList<>();
this.globals = new ArrayList<>();
this.memories = new ArrayList<>();
this.tables = new ArrayList<>();
Collections.addAll(this.functions, imports.functions);
Collections.addAll(this.globals, imports.globals);
Collections.addAll(this.memories, imports.memories);
Collections.addAll(this.tables, imports.tables);
}

public static final class ImportBuilder {
private final Builder builder;
private final String moduleName;
private final String fieldName;

ImportBuilder(Builder builder, String moduleName, String fieldName) {
this.builder = builder;
this.moduleName = moduleName;
this.fieldName = fieldName;
}

public Builder withGlobal(Value value) {
return builder.addGlobal(
new HostGlobal(moduleName, fieldName, new GlobalInstance(value)));
}

public Builder withGlobal(MutabilityType mutability, Value value) {
return builder.addGlobal(
new HostGlobal(
moduleName, fieldName, new GlobalInstance(value), mutability));
}

public Builder withMutableGlobal(Value value) {
return builder.addGlobal(
new HostGlobal(
moduleName,
fieldName,
new GlobalInstance(value),
MutabilityType.Var));
}

public Builder withMemory() {
return builder.addMemory(
new HostMemory(
moduleName, fieldName, new Memory(MemoryLimits.defaultLimits())));
}

public Builder withMemory(MemoryLimits limits) {
return builder.addMemory(new HostMemory(moduleName, fieldName, new Memory(limits)));
}

public Builder withMemory(int initial) {
return builder.addMemory(
new HostMemory(
moduleName, fieldName, new Memory(new MemoryLimits(initial))));
}

public Builder withMemory(int initial, int max) {
return builder.addMemory(
new HostMemory(
moduleName, fieldName, new Memory(new MemoryLimits(initial, max))));
}

public Builder withTable() {
return builder.addTable(
new HostTable(
moduleName,
fieldName,
new TableInstance(
new Table(ValueType.FuncRef, Limits.unbounded()))));
}

public Builder withTable(ValueType t) {
return builder.addTable(
new HostTable(
moduleName,
fieldName,
new TableInstance(new Table(t, Limits.unbounded()))));
}

public Builder withTable(Limits limits) {
return builder.addTable(
new HostTable(
moduleName,
fieldName,
new TableInstance(new Table(ValueType.FuncRef, limits))));
}

public Builder withTable(int min) {
return builder.addTable(
new HostTable(
moduleName,
fieldName,
new TableInstance(new Table(ValueType.FuncRef, new Limits(min)))));
}

public Builder withTable(int min, int max) {
return builder.addTable(
new HostTable(
moduleName,
fieldName,
new TableInstance(
new Table(ValueType.FuncRef, new Limits(min, max)))));
}

// TODO: check - this mechanism is limited by the available primitive functional
// interfaces
public Builder withProcedure(Runnable f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
f.run();
return new Value[] {};
},
moduleName,
fieldName,
List.of(),
List.of()));
}

public Builder withProcedure(Function<Instance, Runnable> consumer) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
consumer.apply(instance).run();
return new Value[] {};
},
moduleName,
fieldName,
List.of(),
List.of()));
}

public Builder withSupplier(IntSupplier f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) ->
new Value[] {Value.i32(f.getAsInt())},
moduleName,
fieldName,
List.of(),
List.of(ValueType.I32)));
}

public Builder withSupplier(Function<Instance, IntSupplier> consumer) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I look at this approach, the less I'm convinced this is the way to go.

public Builder withSupplier(Function<Instance, IntSupplier> consumer)

and

public Builder withSupplier(Function<Instance, LongSupplier> consumer)

are going to be not distinguishable after type erasure.

Currently, I think that leaving only:

public Builder withFunction(Function<Instance, Function<Value[], Value[]>> consumer)

and generating the binding out of real modules with something like: https://github.com/andreaTP/chicory-bindgen-poc is the best bet.

return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
consumer.apply(instance).getAsInt();
return new Value[] {};
},
moduleName,
fieldName,
List.of(),
List.of()));
}

public Builder withSupplier(LongSupplier f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) ->
new Value[] {Value.i64(f.getAsLong())},
moduleName,
fieldName,
List.of(),
List.of(ValueType.I64)));
}

// ... complete me
public Builder withConsumer(IntConsumer f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
f.accept(args[0].asInt());
return new Value[] {};
},
moduleName,
fieldName,
List.of(ValueType.I32),
List.of()));
}

public Builder withConsumer(LongConsumer f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
f.accept(args[0].asLong());
return new Value[] {};
},
moduleName,
fieldName,
List.of(ValueType.I64),
List.of()));
}

// ... complete me
public Builder withFunction(IntUnaryOperator f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
return new Value[] {Value.i32(f.applyAsInt(args[0].asInt()))};
},
moduleName,
fieldName,
List.of(ValueType.I32),
List.of(ValueType.I32)));
}

public Builder withFunction(LongUnaryOperator f) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
return new Value[] {Value.i64(f.applyAsLong(args[0].asLong()))};
},
moduleName,
fieldName,
List.of(ValueType.I64),
List.of(ValueType.I64)));
}

// and now let's see how a fallback might look like
public Builder withFunction(Function<Instance, Function<Value[], Value[]>> consumer) {
return builder.addFunction(
new HostFunction(
(Instance instance, Value... args) -> {
return consumer.apply(instance).apply(args);
},
moduleName,
fieldName,
List.of(ValueType.I64),
List.of(ValueType.I64)));
}
}

public ImportBuilder withNewImport(String moduleName, String fieldName) {
return new ImportBuilder(this, moduleName, fieldName);
}

public Builder withFunctions(List<HostFunction> functions) {
this.functions = functions;
return this;
Expand Down
81 changes: 81 additions & 0 deletions runtime/src/test/java/com/dylibso/chicory/runtime/HostImportsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;

import com.dylibso.chicory.wasm.types.Limits;
import com.dylibso.chicory.wasm.types.MemoryLimits;
import com.dylibso.chicory.wasm.types.MutabilityType;
import com.dylibso.chicory.wasm.types.Value;
import com.dylibso.chicory.wasm.types.ValueType;
import java.util.Arrays;
import java.util.Collections;
import org.junit.jupiter.api.Nested;
Expand Down Expand Up @@ -177,5 +181,82 @@ void addIndex() {
assertEquals(1, result.index().length);
}
}

@Nested
class DSL {

@Test
void withIndex() {
var moduleName = "module";
var fieldName = "filed";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "filed"?

HostImports.builder()
.withNewImport(moduleName, fieldName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this style of nested builder to be strange, especially since only one method can be called. For nested builders, I've found that having a Consumer argument is a good approach, but that doesn't seem useful here. The only thing we are saving by having withNewImport() is the two name arguments.

All of the with methods on Builder replace the entire collection. For example, withGlobals() replaces all of the globals, whereas addGlobal() adds additional globals. So the name withNewImport() seems inconsistent.

Instead, what if we put new methods directly on Builder:

.addGlobal(moduleName, fieldName, Value.i32(1))
.addGlobal(moduleName, fieldName, MutabilityType.Var, Value.i32(1))
.addMutableGlobal(moduleName, fieldName, Value.i32(1))
.addMemory(moduleName, fieldName)
.addMemory(moduleName, fieldName, new MemoryLimits(1))
.addMemory(moduleName, fieldName, 1) // I'd drop these as the MemoryLimits version seems more clear
.addMemory(moduleName, fieldName, 1, 2)
.addTable(moduleName, fieldName)
.addTable(moduleName, fieldName, ValueType.ExternRef)
.addTable(moduleName, fieldName, new Limits(1))
.addTable(moduleName, fieldName, 1) // same, I'd drop these and use the Limits version
.addTable(moduleName, fieldName, 1, 2)
// does type inference work out if we name all of these "addFunction"?
.addProcedure(moduleName, fieldName, () -> System.out.println("hello world"))
.addProcedure(moduleName, fieldName, (Instance inst) -> () -> System.out.println("hello world"))
.addSupplier(moduleName, fieldName, () -> 1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input @electrum !
A few different subjects:

  • add instead of with: agree will change accordingly, thanks for noticing!
  • two name arguments: personally, I feel it awkward to have to type those "names" along with the implementation, the nested builder was an attempt to fix it.
  • new methods directly on Builder: this works for sure, I'm afraid is an improvement less impactful than I originally thought

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it awkward to have to type those "names" along with the implementation

This test might look worse due to repeating the same variables. In actual usage, the field names will need to be unique (and the builder should validate that)? So if we change it to use constants, does it look better?

.addGlobal("test", "g1", Value.i32(1))
.addGlobal("test", "g2", MutabilityType.Var, Value.i32(1))
.addMutableGlobal("test", "gmut", Value.i32(1))
.addMemory("test", "m1")
.addMemory("test", "mlim", new MemoryLimits(1))
.addTable("test", "t1")
.addTable("test", "tref", ValueType.ExternRef)
.addTable("test", "t3", new Limits(1))

Another problem is that the code formatter forces each chained call to be on a separate line. If we were formatting the code by hand, it would look better:

.add("test", "g1").global(Value.i32(1))
.add("test", "g2").global(MutabilityType.Var, Value.i32(1))
.add("test", "gmut").mutableGlobal(Value.i32(1))
.add("test", "m1").memory()
.add("test", "mlim").memory(new MemoryLimits(1))
.add("test", "t1").table()
.add("test", "tref").table(ValueType.ExternRef)
.add("test", "t3").table(new Limits(1))

Which I agree looks slightly cleaner, but we can't format like that...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this!

.add("test", "g1").global(Value.i32(1))

I believe that we should not be limited by our own formatter, and we can always disable it when necessary 😏

.withGlobal(Value.i32(1))
.withNewImport(moduleName, fieldName)
.withGlobal(MutabilityType.Var, Value.i32(1))
.withNewImport(moduleName, fieldName)
.withMutableGlobal(Value.i32(1))
.withNewImport(moduleName, fieldName)
.withGlobal(Value.i32(1))
.withNewImport(moduleName, fieldName)
.withMemory()
.withNewImport(moduleName, fieldName)
.withMemory(new MemoryLimits(1))
.withNewImport(moduleName, fieldName)
.withMemory(1)
.withNewImport(moduleName, fieldName)
.withMemory(1, 2)
.withNewImport(moduleName, fieldName)
.withTable()
.withNewImport(moduleName, fieldName)
.withTable(ValueType.ExternRef)
.withNewImport(moduleName, fieldName)
.withTable(new Limits(1))
.withNewImport(moduleName, fieldName)
.withTable(1)
.withNewImport(moduleName, fieldName)
.withTable(1, 2)
.withNewImport(moduleName, fieldName)
.withProcedure(() -> System.out.println("hello world"))
.withNewImport(moduleName, fieldName)
.withProcedure((Instance inst) -> () -> System.out.println("hello world"))
.withNewImport(moduleName, fieldName)
.withSupplier(() -> 1)
.withNewImport(moduleName, fieldName)
.withSupplier((Instance inst) -> () -> 1)
.withNewImport(moduleName, fieldName)
.withSupplier(() -> 2L)
// this doesn't compile now as expected, since we do not handle Double just
// yet
// .withSupplier(() -> { return 0.0 })
.withNewImport(moduleName, fieldName)
.withConsumer(
(int i) -> {
var x = i + 1;
})
.withNewImport(moduleName, fieldName)
.withConsumer(
(long l) -> {
var x = l + 2L;
})
// this doesn't compile now as expected, since we do not handle Double just
// yet
// .withConsumer((double d) -> { var x = d + 2; })
.withNewImport(moduleName, fieldName)
.withFunction((int i) -> i + 1)
.withNewImport(moduleName, fieldName)
.withFunction((long l) -> l + 2L)
// this doesn't compile now as expected, since we do not handle Double just
// yet
// .withFunction((double d) -> d + 2.0)
.withNewImport(moduleName, fieldName)
.withFunction(
(Instance inst) ->
(Value[] args) -> {
return new Value[] {};
})
.build();
}
}
}
}
Loading