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

Run wabt tools in Chicory #341

Merged
merged 5 commits into from
May 20, 2024
Merged

Run wabt tools in Chicory #341

merged 5 commits into from
May 20, 2024

Conversation

electrum
Copy link
Collaborator

Modification of #337 to avoid problematic shading by splitting out tests into separate modules.

@electrum
Copy link
Collaborator Author

The flakiness mentioned in TestGenMojo seems to happen even with the retries:

Caused by: java.lang.IllegalArgumentException: Resource not found at classpath: wast2json
    at com.dylibso.chicory.runtime.Module.lambda$builder$4 (Module.java:483)
    at com.dylibso.chicory.runtime.Module$Builder.build (Module.java:532)
    at com.dylibso.chicory.wabt.Wast2Json.<clinit> (Wast2Json.java:34)
    at com.dylibso.chicory.maven.TestGenMojo$TestGenerator.generateTests (TestGenMojo.java:232)
    at com.dylibso.chicory.maven.TestGenMojo.lambda$execute$1 (TestGenMojo.java:171)

I'll investigate this later.

@electrum
Copy link
Collaborator Author

There were two problems that caused the flakiness:

  • Static usage of runtime.Module, which is not safe as runtime.Module is mutable. This is something to improve in the Chicory APIs, as it would be nice to only parse a WASM module once.
  • Relying on the thread context ClassLoader to load resources. When running the test generator in parallel, the context ClassLoader was sometimes wrong.


public static byte[] parse(File file) {
Module module = Module.builder(Wat2Wasm.class.getResourceAsStream("/wat2wasm")).build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍

@andreaTP
Copy link
Collaborator

Thanks for all the fixes and the further analysis @electrum , much appreciated!

There are good improvements here over my initial approach.
At the same time, on my machine this is like 2/3 times slower than using the downloaded binaries.
I start to think that we should hold a bit for those modifications and do a round of perf improvements before moving on ...

I'm happy to hear more opinions!

@electrum
Copy link
Collaborator Author

If we’re not going to merge this, then we should revert the previous commit, as Chicory is currently broken in IntelliJ.

The current performance seems acceptable for now. While worse from a relative standpoint, the absolute build time is still fast enough. I’d rather have it working in IntelliJ.

Merging this will provide some good ”low hanging fruit” for performance improvements.

Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

As agreed, this is good to go.

@andreaTP
Copy link
Collaborator

Let's create a Merge commit to preserve your work, and the commits are also well defined.

Thanks for the help!

@andreaTP andreaTP merged commit c3bd389 into dylibso:main May 20, 2024
10 checks passed
@electrum electrum deleted the wabt branch May 20, 2024 19:56
@electrum
Copy link
Collaborator Author

Thanks! I'll look at #344 as that is an obvious performance win for these module reuse cases.

BTW, with the "well defined commits" workflow, the "rebase merge" option is helpful, as it preserves the contents of the original commits while also ensuring a linear history. This is what we use for Trino and other projects.

@andreaTP
Copy link
Collaborator

This is what we use for Trino and other projects.

I used to do it, I switched to "merge squash" as it's a little more "beginner friendly", e.g. you don't have to ask people to rebase etc.
If you have a strong opinion about this detail, I'm not opposed, please go ahead and propose it!
Granted, I can squash my commits 🙂

@electrum
Copy link
Collaborator Author

You can choose which option to use for a specific PR when you merge it. I use both options myself, depending on the PR.

@andreaTP
Copy link
Collaborator

Fair, would you like to streamline this guideline with a sentence or two in Contributing ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants