Native Image Committer and Community Meeting 2024-11-28

[wirthi]

List of all past and upcoming meetings: #3933

Next meeting is planned for December 19, 2024. After that, there will be a meeting on the last Thursday of each month.

New and Noteworthy

• Native Image Layers:
  [GR-59340] Saturate all reachable methods in open world. #9994
  [GR-58118] Adapt saturation to open world. #9962
  [GR-58622] Compact binary encoding for ObjectCopier. #9959 (90%-95% smaller .nil files, much faster builds)
  [GR-55192] Add support for patching prior layer heaps. #10007
  [GR-59809] Filter StringConcatFactory.CACHE to only include reachable types in the image #10146
  [GR-59782] Ensure vtables are calculated for all types when using open world analysis. #10144
  [GR-59911] Persist ImageHeapConstant id across layers #10119
  [GR-58055] Ensure unique fallback methods. #10102
  [GR-59845] Improve loading of base layer elements #10100
  [GR-57390] Enable constant folding on getAllLayers calls. #10090
  [GR-59515] Invokes with 0 callees are not unreachable in open analysis. #10067
  [GR-58518] Register base layer virtual calls as roots. #10061
  [GR-59445] Ensure Word fields are saturated when unavailable. #9995
  [GR-59009] Introduce track across layers flag. #9965
  [GR-59324] Workarounds for layered image reloading quirks. #9953
  [GR-55278] [GR-58056] Collect all external values for Layered Image #9905

• GuestGraal
  [GR-59661] Further refine CustomGraalClassLoader support for libgraal building. #10082
  [GR-59717] Added hook for object reachability notifications to Native Image API. #10075
  [GR-57627] Allow LibGraalFeature class to be loaded by HostedLibGraalClassLoader. #10049
  [GR-59436] Do not use Graal as JIT when running NativeImage driver. #9977

• Streamline reachability metadata
  [GR-58477] Simplify unsafe allocation, field registration and serialization reachability metadata #10178
  [GR-59858] Mark type of hidden field reachable. #10104
  [GR-54953] Options to include all metadata from module or classpath entry #10180

• Usability:
  [GR-59651] Link the troubleshooting guide in the GitHub issue template. #10053
  [GR-57827] Initialize security providers at run time. #10143
  [GR-56112] Add detection of System.getProperty("java.home") calls #10030
  [GR-59074] Promote PGO options on Optimizations and Performance entry page. #9903
  Scan new versions for existing libraries graalvm-reachability-metadata#552
  Add Getting Started guide graalvm/native-build-tools#624

• Monitoring / tools:
  [GR-59924] [GR-59935] Fix JFR-specific substitutions on JDK21. #10129
  [GR-52996] Improve VS detection and related error messages. #10059
  [GR-56601] Initial support for jcmd. #9963 (contributed by @roberttoyonaga from Rad Hat)

• Debugging
  [GR-58932] Update gate to use GDB 14.2. #10112
  [GR-59298] Incorrect compressed oop calculation for hubs in gdb-debughelpers. #9948
  [GR-59073] Adjust debugtests to gdb-debughelpers renaming. #9900

• Performance and Image Size
  [GR-43361] [GR-50205] Whole-Program Sparse Conditional Constant Propagation #9821 Whole-Program Sparse Conditional Constant Propagation (Skipflow) enabled by default in JDK25 (after our branch-off in December). Presentation in the next meeting Dec 19th!
  [GR-58492] Layered NI use open world analysis results as virtual invoke profiles. #9938

• Other
  [GR-58575] SubstrateVM PLT/GOT Feature #9883
  [GR-59326] Ensure ForkJoin commonPool parallelism always respects NativeImageOptions.NumberOfThreads. #9950
  [GR-57608] Support --enable-native-access hosted option in native image #10076
  [GR-51653] Expose method printing format and ability to collect transitive module readers #10166
  [GR-59959] Change package name computation for hybrid objects. #10156
  [GR-59582] Better messages for OutOfMemoryErrors and small cleanups/bugfixes. #10072
  [GR-57090] Add mandatory approvers for substratevm. #10169
  [GR-57270] Use the refactored isolate arguments and cleanup. #9947

Questions

@wirthi the Graal project favors rebasing over merging (to update to newer version). When adopting community PRs that causes troubles (changed commit hashes, sometimes mirroring breaks). How much of a problem is that for the community; previously,we tried not to touch commits (not to change commit hashes) because that was more comfortable to the community.

Deep Dive: JCMD in GraalVM Native Image (@roberttoyonaga )

Summary

An implementation of the JCMD feature is being integrated. This will allow the JCMD tool to attach to a Native Image executable and issue diagnostic commands similar to what's possible in OpenJDK.

Motivation

Native Image has a growing number of features such as NMT, heap dumps, thread dumps, and compilation dumps that rely on handling signals to perform operations requested at runtime. This is not ideal because there is a limited number of signals that are available to be used. It is also not ideal because arguments cannot be easily used with signals. Supporting JCMD would be a better alternative to signals because we can support an unlimited number of commands and also accept arguments.

JFR in OpenJDK heavily uses JCMD to accept user requests at runtime. Currently, there is no way to stop/start/dump recordings spontaneously at runtime (apart from using the remote JMX feature - which has significant overhead with respect to image size). This is inconvenient if a user wants to diagnose a problem without shutting down or restarting their application. For example, if a user wants to access JFR recordings from their application, they need to terminate their application so that a JFR dump is created. With JCMD they are able to dump recordings whenever they desire without needing to restart their entire application.

General Implementation Notes

The JCMD feature relies on two components: the Attach API feature and the DCMD feature. These willa ll be integrated together. Implementation of the Attach API is what allows the JCMD tool to connect to the JVM, issue commands, and receive response data. The DCMD infrastructure is what parses and executes the individual diagnostic commands.

There is a dedicated notification thread which handles Attach API connections and delegates command tasks to the DCMD feature. The Attach API uses unix domain sockets for communication. The Attach API is initialized with a handshake that involves the out of process client sending SIGQUIT and creating a file to indicate that initialization should begin.

Limitations

Not all JCMD diagnostic commands available in OpenJDK will initially be available in Native Image. Please see the documentation PR for an exact list of commands available: #9987

Currently, this feature will not be available on Windows. This is not really a regression since heap dump, thread dump, compilation dump, nmt dump, and JFR, all don't work on Windows anyway (since they require unix signals to operate).

The Windows implementation will be largely independent of code in this posix implementation. So there will be no need to go back and rework anything here.

Links

Internal PR #9963
Original PR #9232
Documentation PR #9987

[zakkak]

Regarding:

the Graal project favors rebasing over merging (to update to newer version). When adopting community PRs that causes troubles (changed commit hashes, sometimes mirroring breaks). How much of a problem is that for the community; previously,we tried not to touch commits (not to change commit hashes) because that was more comfortable to the community.

As mentioned in the call, my only concern is whether GitHub PRs will be able to properly track the rebased commits. We already see some PRs appearing as closed despite actually being merged, e.g.:

• [GR-59340] Saturate all reachable methods in open world. #9994
• [GR-58622] Compact binary encoding for ObjectCopier. #9959
• [GR-55192] Add support for patching prior layer heaps. #10007

Not sure if this is related to rebasing/merging or to some issue with the graalvm-bot though. It's most probably the latter as if I go to 8637bb7 github shows that the commit is actually merged on master. One guess of mine is that when you merge multiple PRs in a single internal merge and then push the changes upstream only one of the upstream PRs ends up getting marked as merged and the others are marked as closed.

Another question is, if you end up rebasing upstream PRs would you also force push to the contributor's branch? To do so contributors need to check the "Allow edits and access to secrets by maintainers" checkbox when opening a PR. Another thing that needs to be taken in consideration is what happens if the contributor force pushes after the rebase? (I guess this is already an issue, since sometimes contributors force push or add new commits after their work has been integrated internally).

cc @vjovanov @wirthi

[christianhaeubl]

We already see some PRs appearing as closed despite actually being merged

Thanks, I forwarded that internally.

Another question is, if you end up rebasing upstream PRs would you also force push to the contributor's branch?

I think it makes sense to keep an unmodified version of the original contribution around. So, no, we would not force push to the contributor's branch. Unfortunately, this makes it even more likely that GitHub loses track. For more complex changes, the result would then probably look like the following:

• the final PR that was merged would have status "Merged"
• the original contributor PR would have status "Closed"
• both PRs would be linked (pretty much like we already do it now, see [GR-56601] Add support for JCMD and the Attach API #9232 for an example)

Would something like that be good enough?

Another thing that needs to be taken in consideration is what happens if the contributor force pushes after the rebase? (I guess this is already an issue, since sometimes contributors force push or add new commits after their work has been integrated internally).

This is already a problem. When I start integrating a PR, I typically post a comment to indicate that no further changes should be pushed to this PR/branch. I think we should establish that as best practice.

[zakkak]

• the final PR that was merged would have status "Merged"
• the original contributor PR would have status "Closed"
• both PRs would be linked (pretty much like we already do it now, see [GR-56601] Add support for JCMD and the Attach API #9232 for an example)

Would something like that be good enough?

Does this mean that for rebased changes a new PR will be mirrored on GitHub and a comment will be added to the both PRs indicating the relationship between the two? If so it should be good enough.

This is already a problem. When I start integrating a PR, I typically post a comment to indicate that no further changes should be pushed to this PR/branch. I think we should establish that as best practice.

👍

[christianhaeubl]

Does this mean that for rebased changes a new PR will be mirrored on GitHub and a comment will be added to the both PRs indicating the relationship between the two?

Yes, a new PR will be mirrored for the rebased changes.
Thanks, then let's try this approach for one of the next contributions.

[zakkak]

Another instance were @olpaw had to manually close the issue?

#10161

I also forgot to mention on my original comment that when GitHub fails to properly mark the PR as closed, the commit also doesn't link back to the PR as shown below:

[christianhaeubl]

@ansalond Is this scenario covered by the already existing issue?

[ansalond]

Another instance were @olpaw had to manually close the issue?
#10161

I only had a quick look, but from what I can see I believe that these commits were squashed manually into 7d964fa before merging the upstream PR. I can see how that might confuse GitHub, but we do not have a ticket to prevent that.

I also forgot to mention on my original comment that when GitHub fails to properly mark the PR as closed, the commit also doesn't link back to the PR as shown below

Indeed, that link appears only when the corresponding GitHub PR is merged rather than closed. In your example:

• closed: [GR-43837] Enable --report-unsupported-elements-at-runtime by default and deprecate the option. #8815
• merged: [GR-53334] Refactor tags into tag groups. #8913

We have an internal ticket to change our mirroring so that PRs appear as merged, but I cannot provide an ETA.