8247666: Support Lambda proxy classes in static CDS archive #364
Conversation
|
/label remove hotspot |
|
/label add hotspot-runtime,core-libs |
|
👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into |
|
@calvinccheung The |
openjdk
bot
added
hotspot-runtime
|
@calvinccheung The |
Webrevs
|
There was a problem hiding this comment.
@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 233
@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 355
It seems very fragile to require listing the CP index to invokedynamic entries of a class file. Have you considered a simpler usage model without CP indices and default to all invokedynamic to LambdaMetaFactory?
| */ | ||
| public static native boolean isDynamicDumpingEnabled(); // will return false for static dumping. | ||
| public static native boolean isCDSDumpingEnabled(); |
There was a problem hiding this comment.
I suggest to rename CDS::isCDSDumpingEnabled to CDS:isDumpingEnabled as this method is a static method in CDS case and the word CDS in the method name is just noise.
JVM entry point JVM_IsCDSDumpingEnabled is good.
There was a problem hiding this comment.
Fixed in webrev 03.
src/hotspot/share/prims/jvm.cpp
Outdated
| JVMWrapper("JVM_IsDynamicDumpingEnable"); | ||
| return DynamicDumpSharedSpaces; | ||
| JVM_ENTRY(jboolean, JVM_IsCDSDumpingEnabled(JNIEnv* env)) | ||
| JVMWrapper("JVM_IsCDSDumpingEnable"); |
There was a problem hiding this comment.
typo: missing d s/Enable/Enabled/
There was a problem hiding this comment.
Fixed in webrev 03.
…a-proxy in the classlist: @lambda-proxy: LambHello run ()Ljava/lang/Runnable; ()V 6 LambHello lambdabash ()V ()V 2. Removed BadCPIndex.java; added LambdaProxyClassList.java test. 3. Mandy's comments.
|
Hi Mandy, |
| #include "classfile/resolutionErrors.hpp" | ||
| #include "classfile/symbolTable.hpp" | ||
| #include "classfile/systemDictionary.hpp" | ||
| #include "classfile/systemDictionaryShared.hpp" |
There was a problem hiding this comment.
Are all the new includes necessary?
There was a problem hiding this comment.
Those new includes are not needed. I've removed them.
| classlist_file->print_cr(""); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think if two threads try call ArchiveUtils::log_to_classlist at the same time, the output may be interleaved.
classlist_file is a fileStream, which uses fwrite to write to the file. In theory, if you write the whole line at once, the output should be thread safe (at least on POSIX and Windows). But in your case, you would need to first get the whole line into a buffer, and then write it out at once.
I think it would be safer, and more convenient, to use a lock to ensure thready safety. Maybe we can convert all uses of classlist_file->print() to something like
class ClassListWriter {
static fileStream* _classlist_file;
MutexLocker locker;
public:
outputStream* stream() {
return _classlist_file;
}
static bool is_enabled() {
return _classlist_file != NULL && _classlist_file->is_open();
}
ClassListWriter() : locker(Thread::current(), ClassListFile_lock) {}
static void init() {
// classlist_file init code from ostrea.cpp
}
};
// WAS if (DumpLoadedClassList != NULL && classlist_file->is_open()) {
if (ClassListWriter::is_enabled()) {
ClassListWriter w;
w->stream()->print("aaaa");
w->stream()->print("bbbb");
w->stream()->cr();
}
There was a problem hiding this comment.
I've added the ClassListWriter.hpp and changed other classes which operate on the classlist.
| @@ -4208,6 +4208,11 @@ void InstanceKlass::log_to_classlist(const ClassFileStream* stream) const { | |||
| bool skip = false; | |||
| if (is_shared()) { | |||
| assert(stream == NULL, "shared class with stream"); | |||
| if (is_hidden()) { | |||
| // Not including archived lambda proxy class in the classlist. | |||
There was a problem hiding this comment.
I think it's clearer to say // Don't include archived lambda proxy class in the classlist.
| */ | ||
| public static native boolean isDynamicDumpingEnabled(); // will return false for static dumping. | ||
| public static native boolean isDumpingEnabled(); |
There was a problem hiding this comment.
I think it will be more consistent if we use the same pattern as CDS::isDumpingClassList()
private static final boolean isDumpingArchive;
static {
isDumpingClassList = isDumpingArchive0();
}
/**
* Is the VM writing to a (static or dynamic) CDS archive.
*/
public static boolean isDumpingArchive() {
return isDumpingArchive;
}
Then in LambdaProxyClassArchive.java, there's no need to keep a separate variable of dumpArchive. You can simply call CDS.isDumpingArchive(). The JIT compiler will automatically inline the call so it will be just as fast.
LambdaProxyClassArchive::sharingEnabled should also be rewritten to use this pattern.
There was a problem hiding this comment.
I've changed the API's in CDS.java to be consistent with CDS.isDumpingClasslist.
Can you explain the format? |
|
It is a symbolic representation of a invoke dynamic constant pool entry. An invoke dynamic constant pool of the above program is: The info included in the class list are: |
1. Added the ClassListWriter class to make writing to a classlist file thread safe; 2. Changed API's in CDS.java to be more consistent with CDS.isDumpingClasslist.
|
@calvinccheung this pull request can not be integrated into git checkout 8247666
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
removed
the
merge-conflict
2. Enclose some debug statements with DEBUG_ONLY.
|
Since the class list file is not intended for users to edit/modify but rather a configuration file given when -Xshare:dump is used, the format of BTW: for the method handle reference kind, does the entry have the value (e.g. 6) or the name ("REF_invokeStatic")? I see both formats are described in your different replies.
|
|
For the reference kind, the first format (with number '6') will be stored in the classlist. |
|
@calvinccheung This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 37 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
added
integrated
|
@calvinccheung Since your change was applied there have been 37 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 74ac77e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Following up on archiving lambda proxy classes in dynamic CDS archive (JDK-8198698), this RFE
adds the functionality of archiving of lambda proxy classes in static CDS archive.
When the -XX:DumpLoadedClassList is enabled, the constant pool index related to LambdaMetafactory that are resolved during application execution will be included in the classlist.
The entry for a lambda proxy class in a class list will be of the following format:
@lambda-proxy: <classname> <cp index>e.g.
@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 233@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 355When dumping a CDS archive using the -Xshare:dump and -XX:ExtraSharedClassListFile options, when the above
@lambda-proxyentry is encountered while parsing the classlist, we will resolve the corresponding constant pool indices (233 and 355 in the above example). As a result, lambda proxy classes will be generated for the constant pool entries, and will be cached using a similar mechanism to JDK-8198698.During dumping, there is check on the cp index and on the created BootstrapInfo using the cp index. VM will exit with an error message if the check has failed.
During runtime when looking up a lambda proxy class, the lookup will be perform on the static CDS archive and if not found, then lookup from the dynamic archive if one is specified.
(Only name change (IsDynamicDumpingEnabled -> IsCDSDumpingEnabled) is involved in the core-libs code.)
Testing: tiers 1,2,3,4.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/364/head:pull/364$ git checkout pull/364