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

Upgrade to GraalVM 22.3.0 #3663

Merged
merged 57 commits into from
Nov 23, 2022
Merged

Upgrade to GraalVM 22.3.0 #3663

merged 57 commits into from
Nov 23, 2022

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Aug 23, 2022

Pull Request Description

Upgrading to GraalVM 22.3.0.

Important Notes

  • Removed all deprecated FrameSlot, and replaced them with frame indexes - integers.
    • Add more information to AliasAnalysis so that it also gathers these indexes.
  • Add quick build mode option to native-image as default for non-release builds
  • graaljs and native-image should now be downloaded via gu automatically, as dependencies.
  • Remove engine-runner-native project - native image is now build straight from engine-runner.
    • We used to have engine-runner-native without sqldf in classpath as a workaround for an internal native image bug.
  • Fixed chrome inspector integration, such that it shows values of local variables both for current stack frame and caller stack frames.
    • There are still many issues with the debugging in general, for example, when there is a polyglot value among local variables, a NullPointerException is thrown and no values are displayed.
  • Removed some deprecated native-image options
  • Remove some deprecated Truffle API method calls.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@JaroslavTulach JaroslavTulach marked this pull request as draft August 23, 2022 08:57
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

I'm happy with this as long as it works and does not cause performance regressions :)

@JaroslavTulach
Copy link
Member Author

CI run shows that the DebuggingEnsoTest is failing. Local variables aren't visible as the default visualization doesn't work for int based variable indexes. That's going to take a time, but it is an opportunity to display variable values properly.

@JaroslavTulach
Copy link
Member Author

Formatting CI check seems to be using GraalVM 21.3 instead of 22.1. Ccing @mwu-tow ...

@jtulach
Copy link

jtulach commented Aug 23, 2022

CI run shows that the DebuggingEnsoTest is failing. Local variables aren't visible as the default visualization doesn't work for int based variable indexes. That's going to take a time, but it is an opportunity to display variable values properly.

Right. The reason why this particular test does not work is that the default implementation of NodeLibrary ( DefaultfNodeExports$DefaultScope.readMember ) reads only auxiliary slots from the frame, but ignores all the indexed slots. For, now deprecated, FrameSlot it was OK, because the default implementation just traversed all the FrameSlots. We have to correctly implement this behavior by exporting scope messages from NodeLibrary in BaseNode or ExpressionNode. The getScope message would return a TruffleObject representing a scope that would export all member and scope messages from InteropLibrary, where there are parent scopes, among other things.

I suggest we ignore, or provide a quick fix, for this test for now, and create a follow-up issue to implement variable displaying in the Truffle debugger properly.

@mwu-tow
Copy link
Contributor

mwu-tow commented Aug 23, 2022

@JaroslavTulach

Formatting CI check seems to be using GraalVM 21.3 instead of 22.1. Ccing @mwu-tow ...

The workflow has hardcoded GraalVM version, it needs to be bumped as well.

You don't need to do this for my build-script-based workflows, as the build script parses build.sbt to figure out proper GraalVM version.

@Akirathan Akirathan changed the title Upgrade to GraalVM 22.1.0 Upgrade to GraalVM 22.3.0 Oct 25, 2022
@JaroslavTulach
Copy link
Member Author

Checking the failures is see two:

Great job! Looks like you'll be able to merge soon.

Removed spurious invocations.
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Some suggestions but LGTM otherwise. Also, slightly changed sbt to avoid multiple installations of native-image

Akirathan and others added 5 commits November 17, 2022 12:26
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
…org/enso into wip/enso/UpdateGraalVM22_182845176
- Display local variables from current method.
- Display local variables of callers.
@Akirathan Akirathan force-pushed the wip/enso/UpdateGraalVM22_182845176 branch from 42ae57d to 7283444 Compare November 21, 2022 15:36
@Akirathan Akirathan force-pushed the wip/enso/UpdateGraalVM22_182845176 branch from 139cb9c to 8581038 Compare November 22, 2022 16:37
Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

I seems that each GraalVM update is becoming increasingly harder. Happy that it is finally done ❤️

project/DistributionPackage.scala Outdated Show resolved Hide resolved
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

nits

@@ -63,7 +79,11 @@ public void setSourceLocation(int sourceStartIndex, int sourceLength) {
*/
@Override
public SourceSection getSourceSection() {
return EnsoRootNode.findSourceSection(getRootNode(), sourceStartIndex, sourceLength);
if (this instanceof ExpressionNodeWrapper wrapper) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a isWrapper method that does this instance check and it seems that in unwrapDelegate we don't mind casting the node so maybe stick to one style?

Copy link
Member

Choose a reason for hiding this comment

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

Probably. But then I don't get this fancy Java pattern matching instanceof. Nevertheless, I would not bother with this code that much, as it will probably be rewritten within a few weeks, once I get to a more proper support for debugging.

}
}

lazy val installNativeImage = taskKey[Unit]("Install native-image GraalVM component")
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic already is present in the RRust build script, if you use it to enter sbt shell then you'll have native-image installed.

Copy link
Member

Choose a reason for hiding this comment

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

I have just recently noticed that. Nevertheless we still need installGraalJs, as it is not shipped by default in the latest GraalVM release. Moreover, this task does not install anything, if native-image is already installed. The same is true for installGraalJs. Besides, I don't know how other engine devs, but I don't usually use the rust build script, I just use sbt, which makes these two tasks very convenient.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Nov 23, 2022
@mergify mergify bot merged commit 402ebb2 into develop Nov 23, 2022
@mergify mergify bot deleted the wip/enso/UpdateGraalVM22_182845176 branch November 23, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants