[RFC]: Integrate jdk8u-jfr-incubator with jdk8u-dev
Andrey Petushkov
andrey at azul.com
Thu Feb 6 15:14:24 UTC 2020
Hi Aleksey, All,
Thank you so much for such thorough review! Please see some comments and
answers inline
On 05.02.2020 14:39, Mario Torre 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.
should not be there. anyway these changes are not present in the updated
patch
>> *) 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.
right, this is bug induced by going back and forth on having 8202353
backported as part of initial JFR backport batch.
should be removed
>
>> *) 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.
yes, this was strategical mistake made during initial backport. anyway,
the work on doing this
the right way is almost complete ([1])
>
>> *) 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?
ON_CLASS_CREATION modifies the value of it's InstanceKlass* argument
because JFR
rewrites the class itself if it's subject to this (it happens for all
JFR event classes).
see jfrEventClassTransformer.cpp for more details
>> *) 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.
right, definitely a bug. sorry for that
>
>> *) 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
fix for failed JFR jtreg test, despite the bug is in non-JFR code
>> src/share/vm/services/memTracker.hpp
>> src/share/vm/utilities/bitMap.inline.hpp
these changes are needed for the build with JFR to succeed. maybe some
of upstream
changesets could have been ported to make it clearer, but I really doubt
one would like
to see all accompanying luggage
>> 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.
yes, should be #if
>
>>> 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.
right. and given that we are to remove "trace" things it's likely
removal has more sense
>> *) 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
Regards,
Andrey
[1]
https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-December/010819.html
More information about the jdk8u-dev
mailing list