-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Provide official API for commonly used svm.jar implementation details #4862
Comments
In addition to the above here is a list of |
@kristofdho we discussed your suggestions:
As mentioned before use e.g.
Please copy whatever you need from there into your source base. We have no intention making theses helper methods public API.
This is an interesting usecase.
that will allow you to do that. Thanks for bringing this up.
There are
and
in the API. The only thing left is to add API that allows you to query annotations without triggering JDK internal annotation handling. We have that internally since
This needs to be fixed as well:
|
@zakkak from the reported
They will be added soon (GR-40904). For the other option-use you mentioned there are the following remarks:
|
Thank you @olpaw
This is only used with GraalVM < 22.2 so there is no issue moving forward.
It seems like this was based on some past evaluation of the then options and needs to be re-evaluated. There are people working on it.
@galderz it looks like this was introduced by you in quarkusio/quarkus#13376. Can you please have a look?
The above have been replaced in quarkusio/quarkus#27783
The option adding this flag has been deprecated in quarkusio/quarkus#27783
This was introduced back in Dec 2018, when isolates where spawned by default(?). We should mark this as deprecated and eventually remove it. Users (and testers) wil still disable full stack traces using
@Sanne this was introduced as an option in quarkusio/quarkus@8d16a42 do you remember why it was needed for? |
[Regarding -H:-StackTrace I've commented on https://github.com/quarkusio/quarkus/issues/27784#issuecomment-1241806173 ] Unrelated to the -H options, I'm more concerned about the move of the annotations. I'm understanding that many annotations are being moved from the However it would seem that some of them have a different package; as we're currently building Quarkus with the old annotations (we have to until GraalVM releases this), this is compiling fine (no errors, no warnings), but then the new GraalVM version isn't going to apply these in practice. Examples: This concerns me as we can only observe the problem via obscure side-effects; could we maintain the existing annotations in the current package in addition to the new one, deprecate the old ones, and have the compiler take into account both flavours? Alternatively, may I suggest to only keep them in the original package, mark them as deprecated (so to express they are still not meant as stable API) and address this problem in the future. Essentially postpone the move to a new package and treat that as a separate task to be addressed in a different version, that would keep the current migration simpler and give some more time to consider these implications. |
I agree that we should just copy whatever util function that we use, but that only covers the usages of We're still doing calls like: PlatformNativeLibrarySupport.singleton().addBuiltinPkgNativePrefix("some_prefix");
NativeLibrarySupport.singleton().preregisterUninitializedBuiltinLibrary("library");
NativeLibraries nativeLibraries = ((FeatureImpl.BeforeAnalysisAccessImpl) a).getNativeLibraries();
nativeLibraries.addStaticJniLibrary("library");
nativeLibraries.addStaticNonJniLibrary("otherLibrary");
nativeLibraries.addDynamicNonJniLibrary("anotherLibrary");
I'm probably missing something here, but are you suggesting the functionality I need is already available here?
So it can be used like: registerAnnotatedClassReachabilityHandler(MyFeature::annotatedClassReachable, javax.inject.Singleton.class); Possibly extend it to allow searching for any annotated element:
I don't follow exactly how |
Side question - could we get/generate concise API doc/html with versions and since/deprecated notion in them assuming that list of those |
@kristofdho regarding:
Are you sure this does not work with the new I tested the following: Implemented if (addBundle) {
Module module = ModuleLayer.boot().findModule("jdk.compiler").orElseThrow();
String bundleName = "com.sun.tools.javac.resources.javac";
System.out.printf("Adding ResourceBundle %s:%s%n", module.getName(), bundleName);
RuntimeResourceAccess.addResourceBundle(module, bundleName);
System.out.printf("ResourceBundle added%n");
addBundle = false;
} Then in the Module compilerModule = ModuleLayer.boot().findModule("jdk.compiler").orElseThrow();
String bundleName = "com.sun.tools.javac.resources.javac";
ResourceBundle bundle = ResourceBundle.getBundle(bundleName, compilerModule);
System.out.println(bundle);
bundle.getKeys().asIterator().forEachRemaining(System.out::println); This works as expected and gives me
at image runtime. |
Also works if I use ModuleLayer.boot().findModule("java.xml") and
or
@kristofdho, can you provide me with a sample program where you cannot use |
@olpaw I was merely reporting our usages of internal API and why they are used. I tried removing the So we can probably assume this is no longer an issue. (The internal usage I refer to can probably also be cleaned up a bit, and no longer call |
Thanks for confirming! |
This goes back to quarkusio/quarkus@223288a in 2018. At the time the default GC policy (adaptive) was deemed to generate full GCs constantly compared to say the space/time one. I'm exploring if this is still the case today, and see what tuning can be done, if any, to improve things while staying with the default adaptive GC policy. |
@christianwimmer please could you check #4862 (comment) as well - I think it was missed. |
@kristofdho if you use
with |
@kristofdho this implemented and merged (will be in 22.3): #4945 |
For the record, I've discussed this particular case at the face to face in Zurich with @christianwimmer and I think our conclusion was that it's not a big problem; if something could be done to catch people relying on the old annotations (now silently ignored) that would be a "nice to have", but not a blocker. |
@olpaw We have identified an additional internal API usage, which sort-of fits in the bucket of using the options directly: File imagePath = NativeImageGenerator.generatedFiles(HostedOptionValues.singleton()).toFile();
// This ultimately boils down to the value of SubstrateOptions.Path We use this to find where the image will be writen. Would it be possible to expose the parent directly where these files will get writen? PS: from a library dev standpoint, it may be usefull to also expose |
I also don't think the |
@kristofdho we need to fully understand your need to use those low-level classes. There is
If you need more than that please explain your specific usecase in detail on a separate issue. |
@olpaw I'm honestly not sure if I need more than that or not. We're using it to statically link in jni libraries, one example is the fontmanager library: PlatformNativeLibrarySupport.singleton().addBuiltinPkgNativePrefix("sun_font");
PlatformNativeLibrarySupport.singleton().addBuiltinPkgNativePrefix("sun_java2d_loops_DrawGlyphList");
PlatformNativeLibrarySupport.singleton().addBuiltinPkgNativePrefix("sun_awt_Win32FontManager_getFontPath");
PlatformNativeLibrarySupport.singleton().addBuiltinPkgNativePrefix("sun_awt_Win32FontManager_populateFontFileNameMap0");
NativeLibraries nativeLibraries = ((FeatureImpl.BeforeAnalysisAccessImpl) a).getNativeLibraries();
NativeLibrarySupport.singleton().preregisterUninitializedBuiltinLibrary("fontmanager");
NativeLibrarySupport.singleton().preregisterUninitializedBuiltinLibrary("freetype");
nativeLibraries.addStaticJniLibrary("fontmanager", "awt");
nativeLibraries.addStaticNonJniLibrary("freetype");
nativeLibraries.addDynamicNonJniLibrary("gdi32"); This works in the same way as other JNI libraries are configured in GraalVM already ( |
@olpaw I have discovered a few more API usage cases that were hidden by configuration to allow the previously fixed internal usages:
|
At some point we want to get rid of pattern-based resource specification even in the json-file approach. The problem is that allowing patterns forces the builder to scan through the entirety of classpath and modulepath to determine for each individual entry in there if it matched the given pattern. It is a scalability issue. But feel free to implemented that scanning through your resources on your side (e.g in your feature-code) and then call addResources for each element that you found. |
Would a
also do the trick for your usecase? |
Are you relying on the output in file Allowing users to refer to arbitrary files in feature code and make them part of what constitutes the build-output is problematic and will conflict with replay-bundles. See #5473 |
Re: #4862 (comment)
It's not. But I don't think we want to make this official API as this goes way beyond what we want our users to do with public API (remember, we have to support public API). In your case, I guess, it makes more sense to either, 1) keep using our internals or 2) contribute your features to our codebase so that using internals is fine. cc @christianwimmer @wirthi |
That was the obvious alternative, but we didn't do that for now as there was an internal API for that. We'll implement this on our side instead.
Unless I'm missing something, that would be a build-time only argument. What I'm looking for (and how it is used in the Micronaut NettyFeature) is a way to configure the default value of the property for the runtime of the native executable.
Currently no. At this time we are blindly copying over all
I'm certainly willing to do so, I however currently only have a working setup for Windows, and I remember from previous interactions (years ago at this point) that cross-platform support is preferred over single-platform support. And specifically for the There are other issues that discuss or relate to such a potential API: Edit: I do believe a first version of any native library support will likely start as a custom feature before it can reach a point where it can be made into a PR to include support for everyone. So without official API it makes the road towards general support ever so slightly more cumbersome. |
I see. It makes sense to add an API for that. |
Created #5557 for that. |
Additional issue with contributing our In general anything awt related is a mess with javafx because we need the shim dlls, but substrate doesn't create them. So we need a second dummy project just to create the shims through the official build tools and provide a boatload of additional linker flags to substrate to make it work. |
I don't see any obvious open issue here. #5557 that Paul created has been closed already. Closing this ticket; if there is something concrete left to be done, please open a new ticket specifically for those parts. |
Copied from #4783 (comment):
We have some usages that I think are not yet covered by #4783 (workarounds are very welcome):
com.oracle.svm.core.SubstrateOptions
SubstrateOptions.Class
, as we have multiple entrypoints within a module, and need to switch config based on which one is used.com.oracle.svm.core.jdk.NativeLibrarySupport
,com.oracle.svm.hosted.c.NativeLibraries
andcom.oracle.svm.core.jdk.JNIRegistrationUtil
JNIRegistrationJava
.com.oracle.svm.core.jdk.Resources#registerResource(java.lang.String, java.lang.String, java.io.InputStream)
fontconfig.bfc
file to the resources, to be used from the substitution.com.oracle.svm.hosted.FeatureImpl.DuringSetupAccessImpl#registerClassReachabilityListener
com.oracle.svm.hosted.classinitialization.ConfigurableClassInitialization
/cc @olpaw
The text was updated successfully, but these errors were encountered: