[RFC]: Integrate jdk8u-jfr-incubator with jdk8u-dev
Andrew John Hughes
gnu.andrew at redhat.com
Tue Feb 4 05:25:03 UTC 2020
On 03/02/2020 19:19, Aleksey Shipilev wrote:
> 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.
First, thanks for looking over this. I haven't had chance myself yet,
but intend to before it goes in.
I agree it should be an RFR, but it's also not a straight-forward code
review, but a merge. Thus, all the changes should (in theory) have
already been reviewed and so, any further changes warrant a new bug,
fixed either in the incubator before integration or after.
>
> 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.
>
Agreed. This looked odd to me in doing the merge.
> *) Not sure why new jdk8u252-b01 is in .hgtags there, also THIRD_PARTY_README changes.
>
I think you'd be better considering the merge changesets, not the webrev:
https://cr.openjdk.java.net/~andrew/openjdk8/jfr/hotspot/merge.changeset
https://cr.openjdk.java.net/~andrew/openjdk8/jfr/jdk/merge.changeset
https://cr.openjdk.java.net/~andrew/openjdk8/jfr/root/merge.changeset
The webrev is based against jdk8u252-b01 so it does pick up some
post-jdk8u252-b01 that are already in the 8u-dev repos. I'm sure you're
familiar with this from our Shenandoah merges.
The webrevs are provided for easier navigation, but the merge changesets
should be the official source on what is actually being changed.
> *) 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.
>
Maybe a cleanup patch is warranted then?
> *) 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?
>
See comments above about the webrev. These are "8233995: java.vm.vendor
(and potentially other properties/fields) not correctly set in
Windows/Hotspot build of OpenJDK8"
> *) 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.
8020701 does seem to be in 8u, so maybe I've misunderstood you here:
https://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot/rev/5e3b6f79d280
>
> *) 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
>
I'll let those who've worked on this comment on the rest.
Thanks,
--
Andrew :)
Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew
More information about the jdk8u-dev
mailing list