[RFC]: Integrate jdk8u-jfr-incubator with jdk8u-dev
Mario Torre
neugens at redhat.com
Thu Feb 6 10:40:54 UTC 2020
I created two bugs for now:
https://bugs.openjdk.java.net/browse/JDK-8238589
https://bugs.openjdk.java.net/browse/JDK-8238590
The first is to track the polishing efforts (only cosmetic, I will
file separate issues for each bug we identify that has is functional
rather than cosmetic).
The second is to pass --enable-jfr by default (in fact, turn it into a
--disable-jfr flag).
Cheers,
Mario
On Wed, Feb 5, 2020 at 12:39 PM Mario Torre <neugens at redhat.com> wrote:
>
> 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
>
--
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