search

Shopify / seafoam

A tool for working with compiler graphs dumped by the GraalVM compiler
MIT License
126 stars 22 forks source link

Version the graph examples #84

Closed nirvdrum closed 5 months ago

nirvdrum commented 5 months ago

This PR restores the script for generating examples and modernizes it.

We were unable to run the Ruby examples with GraalVM 21.2.0 because Graal was missing a necessary option. When those results were generated manually they were done with a patched version of Graal. Subsequent versions of Graal have the option enabled so this script can now process the Ruby examples.

To help keep storage size down, I've switched all the top-level examples to symlinks to a GraalVM-versioned set of files. Now that the Ruby example graphs are generated automatically, I've compressed them just like the Java example graphs.

I think is in keeping with what @chrisseaton had in mind in #37. I haven't yet added the ability to directly compare graphs from different GraalVM versions, although that can be done approximately with the seafoam describe command.

nirvdrum commented 5 months ago

The UUID is unique for each graph. If you regenerate the examples, you'll get a new graph. While it's inconvenient that the one spec would fail, the expectation is that you'll go verify the UUID is correct by other means and ensure the test is checking for the correct value. If we made it a regex pattern it could result in the parser breaking without anyone noticing.

rwstauner commented 5 months ago

I did notice that in 2 different runs I got different values, but I thought if it was always different I would have seen you update it... which I didn't. I do see it has changed a few times in the history.

What does the value mean? Or what is it tied to?

Now I realize that when I run the script I have updates to examples/graalvm-ce-java11-21.2.0 but this PR doesn't change those. If I just restore that dir the spec passes, which explains why you didn't have to update the value in your PR. Just to check my sanity: does the script update your examples/graalvm-ce-java11-21.2.0 dir? Did you just not include them because they didn't need to change (to keep the diff down)?

nirvdrum commented 5 months ago

Now I realize that when I run the script I have updates to examples/graalvm-ce-java11-21.2.0 but this PR doesn't change those. If I just restore that dir the spec passes, which explains why you didn't have to update the value in your PR. Just to check my sanity: does the script update your examples/graalvm-ce-java11-21.2.0 dir? Did you just not include them because they didn't need to change (to keep the diff down)?

I didn't include them because there was no real need to regenerate them. Moreover, we can't fully regenerate them without patching Graal since GraalVM 21.1.0 did not support the --engine.InlineOnly option. Those graphs were generated manually with a patched version of Graal.

I suppose I could just not regenerate the graphs if the directory exists, but if we add new examples or change any of the engine options, we could then miss updates. In either case the person running the tool needs to be aware of what they're pushing. Given the node IDs change, this operation will never be idempotent, so it's incumbent on the person running the script to only commit meaningful changes.

rwstauner commented 5 months ago

Thanks, that makes sense to me 👍