[RFC]: Integrate jdk8u-jfr-incubator with jdk8u-dev

[Aleksey Shipilev]

Hi,

Not sure why this is RFC, while we actually need to RFR this integration very carefully, as it
touches many delicate parts of VM. So, treating this as code review below.

Comments from the cursory look:

On 1/30/20 12:37 PM, Mario Torre wrote:
> https://cr.openjdk.java.net/~andrew/openjdk8/jfr/hotspot/

*) I am a bit concerned about the removal/rename of trace->jfr, as it would probably break some
downstream things, at least for a while. I hope that is the informed risk taking.

*) Not sure why new jdk8u252-b01 is in .hgtags there, also THIRD_PARTY_README changes.

*) makefiles/buildtree.make
   make/linux/makefiles/buildtree.make
   make/solaris/makefiles/buildtree.make
 Indent seems to be missing at new ALWAYS_EXCLUDE_DIRS lines.

*) make/bsd/makefiles/vm.make:
   make/linux/makefiles/vm.make:
   make/solaris/makefiles/vm.make:
  Indent seems to be missing at new EXCLUDE_JFR_PATHS lines.

*) make/windows/makefiles/compile.make:
  How does this change relate to JFR?
  -  349          /D "HS_COMPANY=$(HS_COMPANY)" \
  +  356          /D "HS_COMPANY=$(COMPANY_NAME)" \

*) make/windows/makefiles/defs.make:
  Ditto, how do the hunks around the VENDOR/COMPANY names relate to JFR?

*) make/windows/makefiles/vm.make:
  Ditto, plus hunks around iphlp_interface.obj, vm_version.obj, arguments.obj, etc?

*) src/os/linux/vm/perfMemory_linux.cpp:
  Unclear what new include does there. There are no other changes in this compilation unit! It
should also be stylistically identical to other include lines, e.g. have the space after "#".

*) src/os/posix/vm/os_posix.cpp:
  Unclear what ThreadCrashProtection has to do with JFR. The hunk seems to be coming into jdk/jdk
from JDK-8020701 and rename in JDK-8183925. Neither are listed as backports.

*) src/share/vm/classfile/classLoader.cpp:
   src/share/vm/classfile/systemDictionary.cpp:
  Not sure if we need to repackage the instanceKlassHandle here. Maybe I am missing something?

*) src/share/vm/classfile/systemDictionary.cpp
  The "else" branch near find_or_define_instance_class looks dodgy. I believe it is cleaner to do this:

     // find_or_define_instance_class may return a different InstanceKlass
     if (!k.is_null()) {
       k = find_or_define_instance_class(class_name, class_loader, k, CHECK_(nh));
     }
   #if INCLUDE_JFR
     if (k.is_null() && (class_name == jfr_event_handler_proxy)) {
       ...
     }
   #endif // INCLUDE_JFR

*) src/share/vm/compiler/compileBroker.cpp:
 Stars are misaligned ;)
  2026       const char *failure_reason = ci_env.failure_reason();
  2027       const char* retry_message = ci_env.retry_message();

*) vm/gc_implementation/parallelScavenge/psPromotionManager.inline.hpp:
 It feels safer to stub out implementations like this with INCLUDE_JFR. I would trust compilers to
inline/eliminate the empty method, but would not trust them to eliminate all the method body. So it
feels safer to e.g:

 inline void PSPromotionManager::promotion_trace_event(oop new_obj, oop old_obj,
                                                       size_t obj_size,
                                                       uint age, bool tenured,
                                                       const PSPromotionLAB* lab) {
   #if INCLUDE_JFR
     ...
   #endif
 }

*) src/share/vm/gc_implementation/shared/gcTraceSend.cpp
  Commented out code, starts with "// XXX" at L236?

*) src/share/vm/opto/node.cpp
  New #pragmas related to JFR how?

*) src/share/vm/opto/superword.hpp
  "// JVMCI: OrderedPair is moved up", and this relates to JFR how?

*) src/share/vm/prims/whitebox.cpp:
   src/share/vm/prims/whitebox.hpp:
  Additions of WB_EnqueueInitializerForCompilation, WB_GetHeapAlignment and related rewrite is
related to JFR how?

*) src/share/vm/runtime/biasedLocking.cpp
 I would sleep a bit better, if this was protected by INCLUDE_JFR:

 259   // If requested, return information on which thread held the bias
 260   if (biased_locker != NULL) {
 261     *biased_locker = biased_thread;
 262   }
 263

 ...and:

 502       if (biased_locker != NULL) {
 503         _biased_locker_id = JFR_THREAD_ID(biased_locker);
 504       }

*) src/share/vm/runtime/globals.hpp:
  Not sure what new default arguments for CommandLineFlags::*at are for in this changeset.

*) src/share/vm/runtime/safepoint.cpp:
 Missing EventSafepointCleanupTask for "rotating gc logs", a recent addition to 8u.

*) src/share/vm/runtime/synchronizer.cpp:
 Commented out code:

 1188   // XXX no such counters. implement?
 1189 //  event->set_cause((u1)cause);

*) src/share/vm/services/diagnosticArgument.cpp
   src/share/vm/services/memTracker.hpp
   src/share/vm/utilities/bitMap.inline.hpp
 The changes do not seem related to JFR.

*) src/share/vm/runtime/mutexLocker.cpp
   src/share/vm/runtime/mutexLocker.hpp

 "#ifdef INCLUDE_JFR" is correct, should be "#if INCLUDE_JFR"?


> https://cr.openjdk.java.net/~andrew/openjdk8/jfr/jdk/

*) make/CopyIntoClasses.gmk:
  Why L95..L99 are commented out?

*) make/CreateJars.gmk:
  Indents seem foobared.

*) src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipFileSystem.java
  How is this change related to JFR?

*) test/TEST.groups:
  Double new-line?

*) I have not reviewed the rest of the bulk additions. Those seem to be pretty self-contained.

> https://cr.openjdk.java.net/~andrew/openjdk8/jfr/root/

*) Not sure why new jdk8u252-b01 is in .hgtags there, also THIRD_PARTY_README changes.

---
Thanks,
-Aleksey