Backport reachability-metadata.json parser #23
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
|
@loicottet Thanks! There are some CI failures. Could you please look into those? |
|
Hi @loicottet, is there any progress on this? Thanks |
loicottet
force-pushed
the
lottet/backport-metadata-21
branch
from
237120c
to
11f3306
Compare
|
Thanks for fixing the conflicts @loicottet. I will have a look later today or tomorrow. |
| if (typeDescriptor instanceof NamedConfigurationTypeDescriptor namedDescriptor) { | ||
| String typeName = namedDescriptor.name(); | ||
| ConfigurationType type = configuration.get(condition, typeName); | ||
| ConfigurationType result = type != null ? type : new ConfigurationType(condition, typeName); | ||
| return TypeResult.forType(typeName, result); | ||
| } else { | ||
| return TypeResult.forException(typeDescriptor.toString(), null); |
There was a problem hiding this comment.
I am not able to find this kind of handling in any point in time on master. I assume this was done to prevent applying https://github.com/oracle/graal/pull/8822/files#diff-f6d5768c08225b6ce33958e46c13befe50cbb6d84d9508c52db893a374f65bbf (which IMO is OK to backport in order to avoid deviations in code logic)
There was a problem hiding this comment.
That's correct. My logic on the backport is to keep the changes confined to the parser code as much as possible. We do not want to backport new builder functionality to previous versions, but only allow previous versions to correctly parse newer reachability-metadata.json files.
| private void parseTopLevelObject(EconomicMap<String, Object> obj) { | ||
| Object resourcesObject = null; | ||
| Object bundlesObject = null; | ||
| Object globsObject = null; | ||
| MapCursor<String, Object> cursor = obj.getEntries(); | ||
| while (cursor.advance()) { | ||
| if (RESOURCES_KEY.equals(cursor.getKey())) { | ||
| resourcesObject = cursor.getValue(); | ||
| } else if (BUNDLES_KEY.equals(cursor.getKey())) { | ||
| bundlesObject = cursor.getValue(); | ||
| } else if (GLOBS_KEY.equals(cursor.getKey())) { | ||
| globsObject = cursor.getValue(); | ||
| } | ||
| } | ||
|
|
||
| if (resourcesObject != null) { | ||
| parseResourcesObject(resourcesObject); | ||
| } | ||
| if (bundlesObject != null) { | ||
| parseBundlesObject(bundlesObject); | ||
| } | ||
| if (globsObject != null) { | ||
| parseGlobsObject(globsObject); | ||
| } | ||
| } |
There was a problem hiding this comment.
If I get this right, you need to partially backport globs support for reachability-metadata, but do we really want to backport globs support to resource-config as well?
If we do, shouldn't the oracle/graal@17fc7f0 implementation be preferred?
There was a problem hiding this comment.
Globs can be present in a resource-config.json file in the case where both globs and resource patterns are registered in the same ResourceConfiguration (for example when merging both combined and legacy metadata files). In that case JSON printing falls back to emitting a standalone legacy file instead of integrating resources in reachability-metadata.json (where we want to avoid resource patterns moving forwards), which means we need to be able to emit globs in this legacy file.
If we do, the oracle/graal@17fc7f0 implementation be preferred?
This change applies to JSON emission to ensure that converting legacy resource-config.json files to reachability-metadata.json succeeds most of the time instead of having to fallback to resource-config.json because it contains resource patterns. Since the Java 21 version of Graal doesn't emit reachability-metadata.json files, this optimization is not necessary.
There was a problem hiding this comment.
Globs can be present in a
resource-config.jsonfile
Not in JDK 21 (GraalVM 23.1), that feature was added in JDK 23 (GraalVM 24.1.0). So what I am worried about is backporting the globs support without actually intending to do so.
As you say in #23 (comment):
We do not want to backport new builder functionality to previous versions, but only allow previous versions to correctly parse newer reachability-metadata.json files.
Regarding:
Since the Java 21 version of Graal doesn't emit reachability-metadata.json files, this optimization is not necessary.
I see, what I worry about though is making future backports harder due to deviating from the actual upstream code (which we already do by not fully backporting oracle/graal#8250)
There was a problem hiding this comment.
Not in JDK 21 (GraalVM 23.1), that feature was added in JDK 23 (GraalVM 24.1.0).
But metadata files created on GraalVM 24.1 can be passed to GraalVM 23.1 and are expected to be understood.
So what I am worried about is backporting the globs support without actually intending to do so.
Since globs are meant as a replacement for resource patterns and as such are not purely a new capability of the builder, backporting glob parsing is necessary, otherwise resource handling through reachability-metadata.json would become impossible on 23.1. ResourceConfigurationParser.globToRegex and the Wildcard backport achieve this goal without having to backport the complete glob support (RuntimeResourceSupport.addGlob and the whole logic behind it).
I see, what I worry about though is making future backports harder due to deviating from the actual upstream code (which we already do by not fully backporting oracle/graal#8250)
That's a fair point, keeping parser code as close to master as possible should indeed be a goal. I will start the backporting process for this change and either include it in this PR or open a new one once it's done.
There was a problem hiding this comment.
But metadata files created on GraalVM 24.1 can be passed to GraalVM 23.1 and are expected to be understood.
That would be in the reachability-metadata.json format though, not in the legacy resource-config.json format, right?
If we want to be able to consume "legacy" config files generated by newer GraalVM versions then we need to also support type in reflect-config.json and proxies in serialization-config.json
Since globs are meant as a replacement for resource patterns and as such are not purely a new capability of the builder, backporting glob parsing is necessary, otherwise resource handling through reachability-metadata.json would become impossible on 23.1.
I agree, my only concern is about bringing support for globs in resource-config.json in addition to reachability-metadata.json.
That's a fair point, keeping parser code as close to master as possible should indeed be a goal. I will start the backporting process for this change and either include it in this PR or open a new one once it's done.
Thank you
| public static String globToRegex(String module, String glob) { | ||
| return (module == null || module.isEmpty() ? "" : module + ":") + globToRegex(glob); | ||
| } | ||
|
|
||
| private static String globToRegex(String glob) { | ||
| /* this char will trigger last wildcard dump if the glob ends with the wildcard */ | ||
| String tmpGlob = glob + '#'; | ||
| StringBuilder sb = new StringBuilder(); | ||
|
|
||
| int quoteStartIndex = 0; | ||
| Wildcard previousWildcard = Wildcard.START; | ||
| for (int i = 0; i < tmpGlob.length(); i++) { | ||
| char c = tmpGlob.charAt(i); | ||
| Wildcard currentWildcard = previousWildcard.next(c); | ||
|
|
||
| boolean wildcardStart = previousWildcard == Wildcard.START && currentWildcard != Wildcard.START; | ||
| if (wildcardStart && quoteStartIndex != i) { | ||
| /* start of the new wildcard => quote previous content */ | ||
| sb.append(Pattern.quote(tmpGlob.substring(quoteStartIndex, i))); | ||
| } | ||
|
|
||
| boolean consecutiveWildcards = previousWildcard == Wildcard.DOUBLE_STAR_SLASH && currentWildcard != Wildcard.START; | ||
| boolean wildcardEnd = previousWildcard != Wildcard.START && currentWildcard == Wildcard.START; | ||
| if (wildcardEnd || consecutiveWildcards) { | ||
| /* end of the wildcard => append regex and move start of next quote after it */ | ||
| sb.append(previousWildcard.regex); | ||
| quoteStartIndex = i; | ||
| } | ||
| previousWildcard = currentWildcard; | ||
| } | ||
| /* remove the last char we added artificially */ | ||
| tmpGlob = tmpGlob.substring(0, tmpGlob.length() - 1); | ||
| if (quoteStartIndex < tmpGlob.length()) { | ||
| sb.append(Pattern.quote(tmpGlob.substring(quoteStartIndex))); | ||
| } | ||
| return sb.toString(); | ||
| } | ||
|
|
||
| /** | ||
| * This enum acts like a state machine that helps to identify glob wildcards. | ||
| */ | ||
| private enum Wildcard { | ||
| START("") { | ||
| @Override | ||
| public Wildcard next(char c) { | ||
| return c == '*' ? STAR : START; | ||
| } | ||
| }, | ||
| STAR("[^/]*") { | ||
| @Override | ||
| public Wildcard next(char c) { | ||
| return c == '*' ? DOUBLE_STAR : START; | ||
| } | ||
| }, | ||
| DOUBLE_STAR(".*") { | ||
| @Override | ||
| public Wildcard next(char c) { | ||
| return c == '/' ? DOUBLE_STAR_SLASH : START; | ||
| } | ||
| }, | ||
| DOUBLE_STAR_SLASH("([^/]*(/|$))*") { | ||
| @Override | ||
| public Wildcard next(char c) { | ||
| return c == '*' ? STAR : START; | ||
| } | ||
| }; | ||
|
|
||
| final String regex; | ||
|
|
||
| Wildcard(String val) { | ||
| regex = val; | ||
| } | ||
|
|
||
| public abstract Wildcard next(char c); | ||
| } | ||
|
|
||
| protected void parseGlobsObject(Object globsObject) { | ||
| List<Object> globs = asList(globsObject, "Attribute 'globs' must be a list of glob patterns"); | ||
| for (Object object : globs) { | ||
| parseGlobEntry(object, (condition, module, resource) -> registry.addResources(condition, globToRegex(module, resource))); | ||
| } | ||
| } | ||
|
|
||
| private interface GlobPatternConsumer<T> { | ||
| void accept(T a, String b, String c); | ||
| } |
There was a problem hiding this comment.
This code is new (instead of backporting more code from oracle/graal#8715) in order to reduce the touched classes. Although having less code touched is preferred, this feels quite risky to me as it will only be "tested" in the LTS branches (and probably not to a great extend).
@graalvm/community-maintainers @simonis WDYT? How do you handle similar cases in OpenJDK backports?
There was a problem hiding this comment.
I don't think such a feature would ever be downported in the OpenJDK in the first place :)
In the end it is always a judgement call between the ease of a downport (this one is already quite big) and long term maintainability. I currently can't oversee which existing functionality in the upstream version this new code is replacing and how many additional changes would need to be downorted in oder to make this code look more like the upstream version. If you could detail the alternative (i.e. the additional changes that would need to be downported), that would be helpful.
In general, I have problems to understand the reason behind this downport (see my general comment below). For me, the only justification to accept such a downport would be to maintain compatibility with the corresponding Oracle GraalVM verision 23.1. But in that case we wouldn't have a big choice anyway and should probably accept the exact change that was downported to Oracle GraalVM 23.1 (although we can't see it).
There was a problem hiding this comment.
See #23 (comment) for the reasoning
There was a problem hiding this comment.
I don't think such a feature would ever be downported in the OpenJDK in the first place :)
I see :)
If you could detail the alternative (i.e. the additional changes that would need to be downported), that would be helpful.
For the sake of simplicity (and ease of maintenance), let's say that we would need to backport oracle/graal#8715 as a whole (this would also address my concern in #23 (comment))
In general, I have problems to understand the reason behind this downport (see my general comment below).
Let's keep this discussion in the general comment.
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ResourcesRegistry.java
Outdated
Show resolved
Hide resolved
...le.svm.core/src/com/oracle/svm/core/configure/doc-files/ReflectionConfigurationFilesHelp.txt
Outdated
Show resolved
Hide resolved
...svm.core/src/com/oracle/svm/core/configure/doc-files/SerializationConfigurationFilesHelp.txt
Outdated
Show resolved
Hide resolved
| @Delete // | ||
| static ConcurrentHashMap<Target_java_lang_invoke_MemberName, MethodHandle> LOOKASIDE_TABLE; | ||
| // Checkstyle: resume | ||
|
|
There was a problem hiding this comment.
This seems to be accidentally brought in from oracle/graal#8122. Could you please elaborate on why it's needed?
There was a problem hiding this comment.
Same here, this is a bug that appeared during testing the metadata repository with converted metadata (from *-config.json to reachability-metadata.json). The likely cause is that "type" in reachability-metadata.json brings in the whole class reflection metadata for registered types instead of the more targeted registrations from the legacy files. In this case the bug was already discovered and fixed in oracle/graal#8122, so I included the fix in the backport.
| @@ -53,7 +53,9 @@ protected void registerConditionalConfiguration(ConfigurationCondition condition | |||
| public void flushConditionalConfiguration(Feature.BeforeAnalysisAccess b) { | |||
| for (Map.Entry<String, Collection<Runnable>> reachabilityEntry : pendingReachabilityHandlers.entrySet()) { | |||
| TypeResult<Class<?>> typeResult = ((FeatureImpl.BeforeAnalysisAccessImpl) b).getImageClassLoader().findClass(reachabilityEntry.getKey()); | |||
| b.registerReachabilityHandler(access -> reachabilityEntry.getValue().forEach(Runnable::run), typeResult.get()); | |||
| if (typeResult.isPresent()) { | |||
There was a problem hiding this comment.
Could you please explain why this check is now necessary?
There was a problem hiding this comment.
Testing this PR brought forward the fact that if a class throwing a linkage error when resolved is added to the list of reachability handlers, the build fails since typeResult.get() returns null.
There was a problem hiding this comment.
I see, thank you for the clarification. I wonder if it's worth printing a warning to notify the user that some reachability handlers got ignored (ideally with some info about the LinkafeError, but that won't be trivial I guess).
FTR in the main repo this is fixed by oracle/graal@fff140b
There was a problem hiding this comment.
Thank you for the backport @loicottet.
It looks good except for the schema version updates and 1-2 more changes plus some concerns I have regarding the tradeoff between touching less classes and introducing new code. Let's discuss these and proceed accordingly.
|
@loicottet, @zakkak, I have difficulties in understanding the reason for this quite significant downport. Why is it important that GraalVM 23.1 will partially understand reachability metadata files emitted by GraalVM 24.1 and future releases? Do you expect that popular frameworks/libraries will only emit the new formt in the future? You write "These files are parsed and matched to the existing capabilities of GraalVM 23.1 on a best effort basis, meaning that existing workflows won't be disrupted, but features introduced in subsequent versions will be ignored." What do you mean by "existing workflows won't be disrupted"? From my understanding the reachability metadata is crucial in order for a native application to work correctly. What sense does it make to be able to successfully create a native image with GraalVM 23.1 and metadata in the new format, if the native application in the end won't work correctly (because some features of the metadate were ignored)? In such a case, wouldn't it be preferable that GraalVM 23.1 bails out immediately on the new format, thus forcing dependencies to maintain reachability metadata files in the old format as well, if they want to support GraalVM 23.1? Please let me know if I'm misunderstanding this completely? Also, has this feature been downported to Oracle's GraalVM version 23.1 as well and if yes in which form? I.e. is this downport something we need in the community edition of GraalVM 23.1 in order to maintain compatibility with Oracle GraalVM 23.1? If so, it would be very helpful to know what exact variant of support for the new reachability metadata format landed in OracleVM 23.1 because downporting it in a different way for the community version will probably just cause more confusion than help. |
|
I haven't looked at the patch in detail, yet. That's on my TODO for tomorrow. When talking to @vjovanov at the GraalVM Summit last week he mentioned that there is a set of tests that were run on the Oracle parts. It would be good to include those tests in this downport or as a follow-up. Without it, we'd get even more divergence on what the various metadata parsers would support. Even if there is no observable divergence currently, we need to ensure this doesn't break with later backports so we need a test harness ensuring compatibility. |
Yes that's possible. For frameworks it's quite difficult (and I guess impossible for libraries) to detect the GraalVM version that will eventually be used to build the project, e.g. in Quarkus we support packaging everything in one container and building the native image in another container (in which case the actual GraalVM version can only be obtained after we have done all the config generation). So in order to allow the frameworks and libraries to use the new configuration capabilities without breaking backwards compatibility this backport was proposed. The other alternative, that is already supported is to have frameworks and libraries generate both the new config and the old ones, in this case GraalVM for JDK 21 would ignore the new config, but newer GraalVM versions would parse both configurations and combine them, which seems scary to me. Why not ignore the old configs if a newer one is present? I guess it's to allow for combinations of libraries each using their own configuration scheme, but I will let @loicottet comment on that.
Good point, note that the native image won't work only in very specific cases though, where the configuration is using something without an equivalent in GraalVM 23.1 (e.g. the proxy members registration from oracle/graal#8822). @loicottet I believe that in such cases the compilation should fail, as the application might end up trying to reflectively access these members at run-time.
Yes, that would be an option, but it would most probably result in frameworks and/or libraries not moving to the new configuration to avoid the burden of maintaining both configurations. Which would result in missing out on some new features for users building with newer GraalVM versions and also a much slower adoption of the new configuration scheme, meaning the legacy support would need to be maintained for a longer time.
Yes, that was the ask.
👍 |
|
oracle/graal#7487 mentions that the reachability metadata repository will move to the new format immediately. Hence, the older GraalVM versions need to accept glob patterns as well. |
Thanks for the reference @jerboaa. Note that in the "Backward Compatibility" sections it mentions that:
So it looks like we might need to do some additional backporting. @vjovanov @loicottet can you please confirm? Is glob support in
So the new format support in |
Yes, this backport is the same that was backported to Oracle GraalVM 23.1, including glob parsing support.
We want to avoid having to ship two parallel metadata formats alongside each other, as this would be confusing, and keeping the versions in sync would also be a burden on library owners. Our idea is to introduce The differences between the old and new formats are usually solved in a straightforward manner:
If a program was successfully running on GraalVM 23.1 with legacy configuration files, transforming these files into a |
loicottet
force-pushed
the
lottet/backport-metadata-21
branch
from
11f3306
to
595cd90
Compare
|
@jerboaa I have changed the sentence mentioning the immediate porting of the metadata repo to say that we will wait for the community to adjust to the new changes.
We are completing the test suite now (will take some more time). After that, we will start the discussion about opening this suite. Until then we will run the suite on top of the backport repositories. |
Thanks.
OK. If that suite could be moved to community it would be ideal. Thanks for considering it! |
@loicottet by "including glob parsing support" do I understand correctly that Oracle GraalVM 23.1 includes a backport of oracle/graal#8715 (which is not part of this PR)?
Shouldn't GraalVM print warnings in such cases?
👍, right now there is no good way for us to test this (without creating a similar suite). Even if the suite is not compete it is still better than nothing :) General comment: Please use |
|
For the wider context and for those who didn't had a chance to attend the GraalVM Community Summit I attach @vjovanov great slides about "Native Image Developer Experience" which motivate and explain some of the changes in this PR. |
But doesn't this mean that a project which migrates to the new |
|
[GR-56101] JSON Metadata Versioning, Backwards Compatibility, and Evolution (i.e. oracle/graal#8534) describes the envisioned forward and backward compatibility requirements for the new metadata format. It mentions "type for proxy and lambda classes oracle/graal#7476, new conditions such as typeReached oracle/graal#7480, and the new resource-config.json format oracle/graal#7487" as the most important upcoming features which will have to be backported. |
Only if the application tries to reflectively access methods or fields of the proxy class, which is supported in the latest GraalVM versions but not in GraalVM 23.1 for JDK 21. So migrating an existing configuration of a 23.1-compatible application shouldn't break anything, the proxy classes will be registered as expected. The problem is for applications developed with a newer GraalVM that might be reflectively accessing members of proxy classes. Such applications are expected to fail at runtime since with the current state of the PR the user doesn't get a warning or error at build time about their configuration partially being ignored. |
Partial backport of oracle/graal#9048 and related PRs:
The goal of this PR is to enable the 23.1 version of GraalVM to correctly parse reachability metadata files emitted by GraalVM 24.1 and future releases. These files are parsed and matched to the existing capabilities of GraalVM 23.1 on a best effort basis, meaning that existing workflows won't be disrupted, but features introduced in subsequent versions will be ignored. For example, GraalVM 24.1 can register members (methods, fields and constructors) of proxy classes for reflection, which was not possible before, and does it via the reflection metadata instead of a separate proxy metadata file. When encountering such metadata, the 23.1 parser will now correctly register the proxy class for runtime instantiation, but won't enable reflective access to the specified members.
The PR is split in two commits to facilitate reviewing. The first one is a simple copy-paste of the parser from master (most of the contents of the com.oracle.svm.core.configure package) and the second contains the modifications required to make the parser fit with the features of 23.1.