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

Add native image plugin, for #480 #481

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Conversation

wetneb
Copy link
Contributor

@wetneb wetneb commented Jan 28, 2024

Attempt to follow https://www.graalvm.org/22.2/reference-manual/native-image/guides/use-native-image-maven-plugin/.

To invoke it you need to use GraalVM 17 (22 is not supported, see #479) and run mvn package -P native.
This generates a native image in target/spork, which remains to be tested…

@monperrus
Copy link
Collaborator

Thanks! Could you also document that in the README?

@slarse WDYT?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (10f2f22) 82.70% compared to head (82bcf25) 82.70%.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #481   +/-   ##
=========================================
  Coverage     82.70%   82.70%           
  Complexity      363      363           
=========================================
  Files            43       43           
  Lines          1769     1769           
  Branches        303      303           
=========================================
  Hits           1463     1463           
  Misses          180      180           
  Partials        126      126           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@slarse
Copy link
Collaborator

slarse commented Jan 28, 2024

Hi @wetneb, thanks for your contribution, this is very cool!

I've downloaded GraalVM 17 from here and used that to build the project. Building used up around 11 GiB of ram and fully taxed my 20 core machine for a good while. So I think it's safe to say that building in CI and uploading a built image automatically is not feasible on the hardware we have at our disposal.

I also ran some spot checks with the built image, and it kind of works. It's definitely very fast, roughly 2 orders of magnitude faster than the JVM version (runs merges in ~.01 seconds instead of ~1 seconds).

Unfortunately, it doesn't behave the same as the JVM version either. Especially conflicts behave differently. Just as one example, this conflict scenario should produce this output

class Cls {
<<<<<<< LEFT
=======
protected
>>>>>>> RIGHT
final static void method() {}
}

But the native image instead produces this

class Cls {
<<<<<<< LEFT
    final void method() {}
=======
    protected static void method() {}
>>>>>>> RIGHT
}

Similarly, this conflict scenario should produces this output:

public class Main {
Integer a = 2;
Integer b = 3;
<<<<<<< LEFT
Object
=======
Integer
>>>>>>> RIGHT
field = Integer.valueOf(
<<<<<<< LEFT
2
=======
3
>>>>>>> RIGHT
);
}

But the native image instead produces this

public class Main {
    Object field = Integer.valueOf(2);
    Integer a = 2;
    Integer b = 3;
    Integer field = Integer.valueOf(3);
}

I'm guessing that these behavioral differences are due to implementation details in GraalVM and the JVM. It may also be that certain types of runtime introspection just doesn't work in a native image.

It'd be interesting to see just how much of a difference there is in behavior. It's very interesting that I've not encountered any crashes, just differences in output. That's kind of a shocker, usually things just blow up when assumptions aren't met. I'm not sure if that's good or bad :D

In any case, it doesn't seem to me like the GraalVM image really works as it should. The massive speedup definitely makes it a worthwhile pursuit to make it work, so if you want to devote more effort into trying to find out where the incompatibilities lie, I'd be all ears on your findings.

@wetneb
Copy link
Contributor Author

wetneb commented Jan 28, 2024

Thank you so much for taking the time to evaluate this!

It's very interesting that I've not encountered any crashes, just differences in output. That's kind of a shocker, usually things just blow up when assumptions aren't met. I'm not sure if that's good or bad

Well it's not reassuring at all about GraalVM I would say… I would also have expected the output to remain the same as long as there is no crash.

In my limited experience with this technology I did encounter cases where it was necessary to add some configuration files to simulate reflection in the native image.

I really hope there is a sensible way to figure out how to write those configuration files such that we are reasonably confident that the output is identical to a stock JVM.

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

@wetneb My pleasure!

Now, even if this isn't working quite right, I wouldn't mind merging your contribution if you add some documentation to the README saying:

  1. How to build the native image
    • Include that you need to install GraalVM 17
  2. Make some prominent notes about what we found here (i.e. that it doesn't quite work)

The fact that it runs at all, and actually does work in a lot of cases, is cool enough for me that we can put it into the mainline branch. And it's so very fast, incredible!

In my limited experience with this technology I did encounter cases where it was necessary to add some configuration files to simulate reflection in the native image.

Now that you mention it, this li'l warning actually did pop up in the native image :)

Missing resource : org/eclipse/jdt/internal/compiler/batch/messages.properties for locale en_US

It's possible that missing config is the cause of the differences, but Spork itself doesn't do any configuration by config file (except logging but that won't affect merging) and the above looks like it's for the compiler frontend.

pom.xml Outdated
Comment on lines 299 to 305
<!-- <execution>
<id>test-native</id>
<goals>
<goal>test</goal>
</goals>
<phase>test</phase>
</execution> -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented it out because the build errors with that, but I think getting it to work might be key to solving the bugs in different outputs :)
Let's remove it for now and add a warning about it in the readme.

@wetneb wetneb marked this pull request as ready for review January 29, 2024 17:08
Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

@wetneb Cool!

Couple of tiny notes in the README, then we're good!

Co-authored-by: Simon Larsén <[email protected]>
Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

That's great, thanks @wetneb! It's nice to showcase this as something that's possible, even if it isn't fully functional.

@slarse slarse merged commit 5597cd6 into ASSERT-KTH:master Jan 29, 2024
4 checks passed
@wetneb wetneb mentioned this pull request Apr 7, 2024
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.

4 participants