On Tue, 13 Oct 2020 23:59:38 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now
>> contains ten commits:
>> - fix minimal vm build issues
>> - Merge branch 'master' into 8247666
>> - Merge branch 'master' into 8247666
>> - 1. Symbolic encoding of lambda proxy resolution. An example of
>> @lambda-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.
>> - Merge branch 'master' into 8247666
>> - exclude archived lambda proxy classes in the classlist
>> - updated systemDictionaryShared.[c|h]pp based on suggestions from Ioi
>> - fix extraneous whitespace
>> - 8247666 (initial commit)
>
> src/hotspot/share/interpreter/linkResolver.cpp line 34:
>
>> 32: #include "classfile/symbolTable.hpp"
>> 33: #include "classfile/systemDictionary.hpp"
>> 34: #include "classfile/systemDictionaryShared.hpp"
>
> Are all the new includes necessary?
Those new includes are not needed. I've removed them.
> src/hotspot/share/memory/archiveUtils.cpp line 321:
>
>> 319: }
>> 320: }
>> 321: }
>
> 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();
> }
I've added the ClassListWriter.hpp and changed other classes which operate on
the classlist.
> src/hotspot/share/oops/instanceKlass.cpp line 4212:
>
>> 4210: assert(stream == NULL, "shared class with stream");
>> 4211: if (is_hidden()) {
>> 4212: // Not including archived lambda proxy class in the classlist.
>
> I think it's clearer to say `// Don't include archived lambda proxy class in
> the classlist.`
Fixed.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 78:
>
>> 76: * Check if CDS dumping is enabled via the DynamicDumpSharedSpaces
>> or the DumpSharedSpaces flag.
>> 77: */
>> 78: public static native boolean isDumpingEnabled();
>
> 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.
I've changed the API's in CDS.java to be consistent with CDS.isDumpingClasslist.
-------------
PR: https://git.openjdk.java.net/jdk/pull/364
