[RFC]: Integrate jdk8u-jfr-incubator with jdk8u-dev
Mario Torre
neugens at redhat.com
Wed Feb 5 11:39:42 UTC 2020
On Mon, 2020-02-03 at 20:19 +0100, 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.
Hi Aleksey,
As Andrew has noted, this was a RFC because those patches have been
already reviewed, that being said, I do really appreciate more eyes,
and in fact I should probably have asked directly for a re-review
instead. Changes and fixes will need to have their own bug id, but this
doesn't matter in this context.
Also, the patch is meant to be applied and tested in case we missed
something, for example I am unable to test the Windows changes (see
below) but of course Azul that helped extensively with the backport did
test those, even so, clearly this is the opportunity for everyone to
help with extensive testing and shout if anything breaks.
> 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.
Well... That's the main JFR change... unless we really want to go with
a difficult to maintain chain of if/defs.
It is a risk and we are aware that it may break something but the API
is internal and with our knowledge isn't used anywhere, so we decided
to go for it.
Note that a variant of this patch-set is being already deployed by some
companies in their JDKs distributions, so we do have some confidence.
Last famous words...
> *) 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.
I'm not sure about those, I think there was something wrong with the
export. I created a new clean patch from the repositories:
http://cr.openjdk.java.net/~neugens/JDK8u-JFR.01/
> *) 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?
Those came in because of the changes in vm_version_ext_x86.cpp, which
are related to JFR.
> *) make/windows/makefiles/vm.make:
> Ditto, plus hunks around iphlp_interface.obj, vm_version.obj,
> arguments.obj, etc?
Those changes are related to this backport:
changeset: 50879:d90c3cbf13df
user: rwestberg
date: Thu Jun 28 15:06:55 2018 +0200
summary: 8003209: JFR events for network utilization
This was part of the initial JFR integration in 11. My understanding
when we did the review was that this make change was needed to have the
build code pick up those files when JFR is enabled. Afaik they are not
present in the original backport though, Andrey should be able to
clarify on this point.
> *) 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 "#".
Right, and I'm not sure about this one, the change comes from the first
change in the backport but they should be only in:
src/hotspot/os/linux/os_perf_linux.cpp
I think this one should go away, I'll file a bug to remove it, thanks
for spotting it.
> *) 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.
Hmm, thanks for noticing this. The ThreadCrashProtection is used by the
JFR backport so this change was ncessary, but I'm now questioning why
we didn't backport the rename patch and instead added the
ThreadCrashProtection on top of the WatcherThreadCrashProtection.
I don't think the change is wrong, but it may cause confusion down the
line.
I think this needs some clarification from Andrey too.
> *) 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?
Can you explain what do you see wrong here, I'm not sure I understand?
> *) 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();
Right, I think those are style thing though, we should probably fix
them but in a new bug?
> *)
> 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:
this makes sense, but it's the current implementation in 11 that came
with the backport (and it's still in 11u, I haven't checked the other
versions yet). I can file a new bug to fix this in 8u though if you
prefer.
> *) src/share/vm/gc_implementation/shared/gcTraceSend.cpp
> Commented out code, starts with "// XXX" at L236?
Right, I think this commented out code should be removed completely. I
wasn't much happy about it when we did the round of reviews but assumed
we would work on that later and must have forgotten. I'll file a Jira
bug for most of those clean-ups you have recommended so we can track
them in one go.
> *) src/share/vm/opto/node.cpp
> New #pragmas related to JFR how?
Hmmm, I don't see this in our backport patch...
> *) src/share/vm/opto/superword.hpp
> "// JVMCI: OrderedPair is moved up", and this relates to JFR how?
This change comes from 8136421, but I think it's a mistake we included
in this backport.
> *) 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?
This was necessary for a number of failing tests, see:
8230707: JFR related tests are failing
> *) 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 }
Makes sense, I'll include this in the "cleanup" bug then.
> *) src/share/vm/runtime/globals.hpp:
> Not sure what new default arguments for CommandLineFlags::*at are
> for in this changeset.
I'm not sure if I understand what you mean, but in this patch JFR is
disabled by default. It was agreed during the committer's workshop on
Monday to actually have it on by default, so I will file another Jira
issue for that.
> *) src/share/vm/runtime/safepoint.cpp:
> Missing EventSafepointCleanupTask for "rotating gc logs", a recent
> addition to 8u.
When I apply my most recent export (see above) I do see a
EventSafepointCleanupTask for rotating gc logs, is that that you need?
> *) src/share/vm/runtime/synchronizer.cpp:
> Commented out code:
>
> 1188 // XXX no such counters. implement?
> 1189 // event->set_cause((u1)cause);
Yeah, as I said some of those changes were tricky, we decided to mark
with comments to work on them later, but this is never a good idea,
isn't it?
> *) 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.
I forgot about the motivation for this one as it's been more than one
year, but I remember I got the same issues during my own attempt at
backporting. I believe this had to do with the jfr headers files not
being correctly pulled in. Andrey can you please comment on this one?
> *) src/share/vm/runtime/mutexLocker.cpp
> src/share/vm/runtime/mutexLocker.hpp
>
> "#ifdef INCLUDE_JFR" is correct, should be "#if INCLUDE_JFR"?
We followed the same pattern has the previous define but I think you
are right and it should be #if INCLUDE_JFR as we don't want to risk
compile this code in case INCLUDE_JFR is defined but false, however now
INCLUDE_JFR is completely undefined if we don't --enable-jfr at build
time. Nonetheless it should probably be fixed as to avoid future bugs.
>
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/jdk/
>
> *) make/CopyIntoClasses.gmk:
> Why L95..L99 are commented out?
I think this should be reverted or removed completely.
> *) make/CreateJars.gmk:
> Indents seem foobared.
Ouch... thanks for noticing.
> *) src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipFileSystem.java
> How is this change related to JFR?
I don't see this in my export, again it may have been caused by the
merge patch?
> *) test/TEST.groups:
> Double new-line?
>
Will add this to the cleanup.
I'll ask the rest of the group working on the JFR to comment on the
issues you pointed out I wasn't fully sure about.
Thanks for the review!
Cheers,
Mario
--
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30 9205 5D7E 4952 3F65 7898
More information about the jdk8u-dev
mailing list