Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Fri, 25 Jun 2021 01:15:29 GMT, Yumin Qi wrote: >> Hi, Please review >> systemDictionaryShared becomes fatter and fatter so it is time to split it >> into functional files. Moved security and jar operation related code into >> CDSProtectionDomain, and moved shared class info (DumpTime/RunTime) to >> sharedClassInfo.[ch]pp, also moved lambda proxy related to >> lambdaProxyClassInfo.hpp. This way systemDictionaryShared.cpp looks neat and >> light. >> >> Tests: tier1,tier3,tier4 >> >> Thanks >> Yumin > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > Remove 'Shared' from class names and rename corresponding files This looks OK to me. The naming of the various classes is still not consistent since the code has evolved for a long time. It was probably a mistake to add `Shared` as a classname suffix, but that was done since the beginning of CDS. Maybe eventually we should rename all the classes with a CDS prefix (similar to the JFR code). That should be done in a separate PR, though. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Mon, 28 Jun 2021 23:13:24 GMT, Ioi Lam wrote: >> Yumin Qi has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove 'Shared' from class names and rename corresponding files > > This looks OK to me. > > The naming of the various classes is still not consistent since the code has > evolved for a long time. It was probably a mistake to add `Shared` as a > classname suffix, but that was done since the beginning of CDS. > > Maybe eventually we should rename all the classes with a CDS prefix (similar > to the JFR code). That should be done in a separate PR, though. @iklam @calvinccheung @erikj79 Thank you for review! - PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Fri, 25 Jun 2021 22:28:50 GMT, Yumin Qi wrote: >> src/hotspot/share/cds/lambdaProxyClassDictionary.hpp line 28: >> >>> 26: #define SHARED_CDS_LAMBDAPROXYCLASSINFO_HPP >>> 27: #include "cds/metaspaceShared.hpp" >>> 28: #include "classfile/javaClasses.hpp" >> >> Is this include necessary? > > It is needed, line 84: > return java_lang_String::hash_code((const jbyte*)sym->bytes(), > sym->utf8_length()); I see. - PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Fri, 25 Jun 2021 16:50:37 GMT, Calvin Cheung wrote: >> Yumin Qi has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove 'Shared' from class names and rename corresponding files > > src/hotspot/share/cds/lambdaProxyClassDictionary.hpp line 28: > >> 26: #define SHARED_CDS_LAMBDAPROXYCLASSINFO_HPP >> 27: #include "cds/metaspaceShared.hpp" >> 28: #include "classfile/javaClasses.hpp" > > Is this include necessary? It is needed, line 84: return java_lang_String::hash_code((const jbyte*)sym->bytes(), sym->utf8_length()); - PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Fri, 25 Jun 2021 01:15:29 GMT, Yumin Qi wrote: >> Hi, Please review >> systemDictionaryShared becomes fatter and fatter so it is time to split it >> into functional files. Moved security and jar operation related code into >> CDSProtectionDomain, and moved shared class info (DumpTime/RunTime) to >> sharedClassInfo.[ch]pp, also moved lambda proxy related to >> lambdaProxyClassInfo.hpp. This way systemDictionaryShared.cpp looks neat and >> light. >> >> Tests: tier1,tier3,tier4 >> >> Thanks >> Yumin > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > Remove 'Shared' from class names and rename corresponding files Latest changes look good. One question. src/hotspot/share/cds/lambdaProxyClassDictionary.hpp line 28: > 26: #define SHARED_CDS_LAMBDAPROXYCLASSINFO_HPP > 27: #include "cds/metaspaceShared.hpp" > 28: #include "classfile/javaClasses.hpp" Is this include necessary? - Marked as reviewed by ccheung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
> Hi, Please review > systemDictionaryShared becomes fatter and fatter so it is time to split it > into functional files. Moved security and jar operation related code into > CDSProtectionDomain, and moved shared class info (DumpTime/RunTime) to > sharedClassInfo.[ch]pp, also moved lambda proxy related to > lambdaProxyClassInfo.hpp. This way systemDictionaryShared.cpp looks neat and > light. > > Tests: tier1,tier3,tier4 > > Thanks > Yumin Yumin Qi has updated the pull request incrementally with one additional commit since the last revision: Remove 'Shared' from class names and rename corresponding files - Changes: - all: https://git.openjdk.java.net/jdk/pull/4568/files - new: https://git.openjdk.java.net/jdk/pull/4568/files/438d54da..1c114bae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4568=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4568=02-03 Stats: 85 lines in 8 files changed: 1 ins; 0 del; 84 mod Patch: https://git.openjdk.java.net/jdk/pull/4568.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4568/head:pull/4568 PR: https://git.openjdk.java.net/jdk/pull/4568
