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

Initialize Builtins at Native Image build time #3821

Merged
merged 5 commits into from
Oct 25, 2022
Merged

Initialize Builtins at Native Image build time #3821

merged 5 commits into from
Oct 25, 2022

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Oct 21, 2022 •
edited
Loading

Pull Request Description

Moved loading of Builtin Types and Methods to a static initializer. That way the information is available at Native Image build time and one does not have to update a corresponding entry in reflect-config which would be a real pain when we start using it in anger.
Fixes https://www.pivotaltracker.com/story/show/183374932

Important notice

For the inliner to do its job, we have to limit call chains. Therefore rather than making clazz.getConstructor().newInstance() calls we should do clazz.getConstructor(). The latter will get constant folded and won't crash when running NI, the former won't.

@hubertp hubertp requested a review from Akirathan October 21, 2022 10:27
@hubertp hubertp changed the title Initialize Builtins at build time Initialize Builtins at Native Image build time Oct 21, 2022
JaroslavTulach
JaroslavTulach approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I am pleasantly surprised that one can Method.invoke without specifying that in the reflect-config.json!

engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Builtins.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Builtins.java Show resolved Hide resolved
...e/runner-native/src/main/resources/META-INF/native-image/org/enso/runner/reflect-config.json Show resolved Hide resolved
@hubertp hubertp force-pushed the wip/hubert/builtins-static-init-183374932 branch from c57f25a to 2550cfc Compare October 24, 2022 09:27
@hubertp
Copy link
Collaborator Author

hubertp commented Oct 25, 2022

This currently breaks because tests call Builtins multiple times and some state is modified during initialization. Resetting the state of types and scope would be recommended but it opens its own can of worms that I don't want to do.

On the other hand, we could statically just initialize the class and then create new instances in Builtins constructor. Unfortunately calls are then not inlined, we get NoSuchMethodException which looks like a limitation of oracle/graal#2500. I will see if I can refactor it a bit so that it is more inliner friendly.

@hubertp
Copy link
Collaborator Author

hubertp commented Oct 25, 2022 •
edited
Loading

On the other hand, we could statically just initialize the class and then create new instances in Builtins constructor. Unfortunately calls are then not inlined, we get NoSuchMethodException which looks like a limitation of oracle/graal#2500. I will see if I can refactor it a bit so that it is more inliner friendly.

Yeap, that was it. As soon as I moved .newInstance() out of clazz.getConstructor().newInstance() in the static initializer to the Builtins constructor, it had no problem with inlining the call. I wish there were more diagnostics telling us that we are hitting some limits because sometimes it feels like walking through a fog with Graal.

@hubertp hubertp force-pushed the wip/hubert/builtins-static-init-183374932 branch from 2550cfc to a464a8f Compare October 25, 2022 09:39
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Oct 25, 2022
@hubertp
Initialize Builtins at build time
52372ea 
Moved loading of Builtin Types and Methods to a static initializer.
That way the information is available at Native Image build time and one
does not have to update a corresponding entry in `reflect-config` which
would be a real pain when we start using it in anger.
@hubertp
changelog
a641b68
@hubertp
Always create new builtin instances
cc2e2e6 
Turns out one cannot re-use builtins that are initialized in the static
context because some state is not reset between tests. Trying to enforce
that will also open a can of worms so we decided against that.

However, while trying to instantiate builtins in the constructor
we would no longer inling constructor calls, resulting in
`NoSuchMethodException` once the native image was generated. This turned
out to be a limitation related to oracle/graal#2500
so that we cannot make calls like `clazz.getContructor().newInstance()`.
`clazz.getConstructor()` in the static initializer and creating new
instances in Builtins constructor was however OK for the inliner and it
was able to procede with constant folding.
@hubertp
nit
4def05d
@hubertp hubertp force-pushed the wip/hubert/builtins-static-init-183374932 branch from 84fe5a1 to 4def05d Compare October 25, 2022 15:39
@mergify mergify bot merged commit 55b9bea into develop Oct 25, 2022
@mergify mergify bot deleted the wip/hubert/builtins-static-init-183374932 branch October 25, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@Akirathan Akirathan Akirathan approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hubertp @Akirathan @JaroslavTulach