-
Notifications
You must be signed in to change notification settings - Fork 62
Wasm gc step1 #1097
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
base: main
Are you sure you want to change the base?
Wasm gc step1 #1097
Conversation
2fd30c9 to
6bc98b5
Compare
tombentley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't claim to understand Wasm GC, but I left a few shallow remarks. If there are specific bits which can/should be checked against the spec then it would be helpful to link to the relevant bits of the spec either in a code or a PR comment.
| private final StructType structType; | ||
| private final FunctionType funcType; | ||
|
|
||
| private CompType(ArrayType arrayType, StructType structType, FunctionType functionType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comptype ::= <functype> | <structtype> | <arraytype>
Do you want to check the exclusivity as precondition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup 👍
| private final ValType valType; | ||
| private final PackedType packedType; | ||
|
|
||
| private StorageType(ValType valType, PackedType packedType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, maybe check that exactly one is null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
|
||
| public boolean equals(FunctionType other) { | ||
| return hashCode == other.hashCode && paramsMatch(other) && returnsMatch(other); | ||
| // For type equivalence, we need structural comparison even when hashCodes differ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means hashCode() can be inconsistent with equals(), which can be pretty unexpected if your objects leak to code that's not written with such inconsistency in mind. So I wonder if it's strictly necessary to couple the Java notion of equality to type equivalence? If not then you could avoid the WTF kind of moments that people have when HashSets don't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is a very fair comment, historically we are inheriting this approach from the previous work on Function References.
I think you are right and we should, eventually, move away from it, I'd give a try to address this in an upcoming PR when dealing with types canonicalization if looks acceptable.
As a remark, we don't expect people to store those in Maps and Sets.
| @@ -0,0 +1,32 @@ | |||
| package com.dylibso.chicory.wasm.types; | |||
|
|
|||
| public final class ArrayType { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other types override equals and hashCode, but not here. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, done 👍
| this.types = List.copyOf(types); | ||
| } | ||
|
|
||
| public FunctionType[] types() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that this returns different types than were passed to the constructor might be worthy of a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair, done 👍
| } | ||
|
|
||
| private static FunctionType parseFunctionType(ByteBuffer buffer) { | ||
| var paramCount = (int) readVarUInt32(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens on int overflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we are not checking some of the limits in place for other runtimes.
The "real world" limit should be this:
| public static final int MAX_FUNCTION_PARAMS = 1000; |
Most(all?) compilers and optimizers are going to respect that limit (alternatively things won't work in the browser).
Instead of fixing this aspect in this PR I'll open a separate issue to introduce more checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened #1154
| var paramsBuilder = new ValType.Builder[paramCount]; | ||
| private static SubType parseSubType(int id, ByteBuffer buffer) { | ||
| if (id == 0x50 | ||
| || // non final typeIdx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the comment apply to the previous line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(autoformatter) should be good now
| t.returns().forEach(this::validateValueType); | ||
| for (var i = 0; i < module.typeSection().typeCount(); i++) { | ||
| var t = module.typeSection().getRecType(i); | ||
| // TODO: fix me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to merge with this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation in this PR barely reaches the Validator, i.e. is not complete.
When completing the Wasm GC implementation those TODO will go away naturally/organically.
| // The following code fixes the 2 tests, but breaks many things in GC: | ||
| // FunctionReferencesType-equivalence.test2 | ||
| // FunctionReferencesType-equivalence.test3 | ||
| // TODO: seems "wrong" when using WasmGC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to merge with this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, this is a work in progress, I prefer to have a few TODOs around to easily get back to the initial thought process for now.
| } | ||
|
|
||
| if (!errors.isEmpty()) { | ||
| errors.stream().forEach(e -> e.printStackTrace()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the the printing just for debugging purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one! Leftover, removed.
|
Thanks a ton @tombentley for the review, much, much appreciated 🙏 I'm about to go on vacation but will address your comments early next year! |
6bc98b5 to
a6660bd
Compare
|
Thanks again for the review @tombentley ! |
Attempting to validate the changes in #1090 I ended up(again) porting most of the work(and some new) already done to this new branch.
It's not "finished" and the encoding of types might change again, but I'm convinced is a decent starting point, and it enables implementing GC opcodes.
The public API is preserved and the test coverage is increased without regressions.
The current plan is: