Re: RFR 8217647: [backport] JFR: recordings on 32-bit systems unreadable
Denghui Dong
denghui.ddh at alibaba-inc.com
Fri Jun 19 02:23:15 UTC 2020
> The changes in the patch look fine. I see you also omitted the changes
> not in 11u. Was this intentional? I compared with the 11u version, so I
> didn't notice this in reviewing the patch itself.
yes, it's intentional. I hope that the jfr code of jdk8u and jdk11u are as consistent as possible, which is good to the later backport work(e.g. 8225797)
> Incidentally, we haven't been building JFR on x86 so far, because it
> crashed when I tried to build it:
> ...
Thanks for sharing.
After I finish the preparation of the environment, I will try to build JFR on the 32bit system and will take a look at this issue.
------------------------------------------------------------------
From:Andrew Hughes <gnu.andrew at redhat.com>
Send Time:2020年6月19日(星期五) 02:00
To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; jdk8u-dev <jdk8u-dev at openjdk.java.net>
Subject:Re: RFR 8217647: [backport] JFR: recordings on 32-bit systems unreadable
On 16/06/2020 04:44, Denghui Dong wrote:
> Could I have a review of this backport?
>
> Original bug:
> https://bugs.openjdk.java.net/browse/JDK-8217647
>
> Original fix from 11u:
> http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/df5487678893
>
> Webrev: http://cr.openjdk.java.net/~ddong/8217647/webrev.00/
>
> Backport reasons:
> 1. Fix the unreadable problem on 32-bit in 8u
> 2. Make subsequent backport more smoothly
>
> The patch cannot apply to 8u cleanly, there are two rejection part and one build failure:
>
> a) Conflict in jfrRecorderService.cpp. Patch wants this:
>
> -static intptr_t write_checkpoint_event_prologue(JfrChunkWriter& cw, u8 type_id) {
> - const intptr_t prev_cp_offset = cw.previous_checkpoint_offset();
> - const intptr_t prev_cp_relative_offset = 0 == prev_cp_offset ? 0 : prev_cp_offset - cw.current_offset();
> +static int64_t write_checkpoint_event_prologue(JfrChunkWriter& cw, u8 type_id) {
> + const int64_t prev_cp_offset = cw.previous_checkpoint_offset();
> + const int64_t prev_cp_relative_offset = 0 == prev_cp_offset ? 0 : prev_cp_offset - cw.current_offset();
> cw.reserve(sizeof(u4));
> cw.write<u8>(EVENT_CHECKPOINT);
> cw.write(JfrTicks::now());
> - cw.write<jlong>((jlong)0);
> + cw.write((int64_t)0);
> cw.write(prev_cp_relative_offset); // write previous checkpoint offset delta
> cw.write<bool>(false); // flushpoint
> - cw.write<u4>((u4)1); // nof types in this checkpoint
> - cw.write<u8>(type_id);
> - const intptr_t number_of_elements_offset = cw.current_offset();
> + cw.write((u4)1); // nof types in this checkpoint
> + cw.write(type_id);
> + const int64_t number_of_elements_offset = cw.current_offset();
> cw.reserve(sizeof(u4));
> return number_of_elements_offset;
> }
>
> ... but the 8u code is:
>
> static intptr_t write_checkpoint_event_prologue(JfrChunkWriter& cw, u8 type_id) {
> const intptr_t prev_cp_offset = cw.previous_checkpoint_offset();
> const intptr_t prev_cp_relative_offset = 0 == prev_cp_offset ? 0 : prev_cp_offset - cw.current_offset();
> cw.reserve(sizeof(u4));
> cw.write<u8>(EVENT_CHECKPOINT);
> cw.write(JfrTicks::now());
> cw.write<jlong>((jlong)0);
> cw.write<jlong>((jlong)prev_cp_relative_offset); // write previous checkpoint offset delta
> cw.write<bool>(false); // flushpoint
> cw.write<u4>((u4)1); // nof types in this checkpoint
> cw.write<u8>(type_id);
> const intptr_t number_of_elements_offset = cw.current_offset();
> cw.reserve(sizeof(u4));
> return number_of_elements_offset;
> }
>
> ... I did a manual adjustment:
> cw.write<jlong>((jlong)prev_cp_relative_offset); => cw.write(prev_cp_relative_offset);
>
> b) Conflict in jfrWriterHost.inline.hpp. Patch wants this:
>
> template <typename BE, typename IE, typename WriterPolicyImpl>
> inline void WriterHost<BE, IE, WriterPolicyImpl>::write(double value) {
> - be_write(*(uintptr_t*)&(value));
> + be_write(*(u8*)&(value));
> }
>
> ... but the 8u code is:
>
> template <typename BE, typename IE, typename WriterPolicyImpl>
> inline void WriterHost<BE, IE, WriterPolicyImpl>::write(double value) {
> be_write(*(u8*)&(value));
> }
>
> ... so I skipped it.
>
> c) Build failure. Patch wants this in jfrRepository.cpp:
>
> + log_info(jfr) ( // For user, should not be "jfr, system"
> + "Unable to recover JFR data");
> + break;
>
> ... but 8u didn't support unified logging, so I added "if (LogJFR) tty->print_cr("Unable to recover JFR data");"
>
>
> d) Other Problem: the original patch(http://hg.openjdk.java.net/jdk/jdk/rev/0abec72a3ac2) also modified jfrChunkRotation.hpp、jfrChunkRotation.cpp and jfrJniMethod.cpp, but the backport patch for 11u ignored them(http://mail.openjdk.java.net/pipermail/jdk-updates-dev/2019-April/000921.html)
>
> Thanks,
> Denghui Dong
>
Thanks for the comprehensive analysis. It's really helpful in reviewing
this.
The changes in the patch look fine. I see you also omitted the changes
not in 11u. Was this intentional? I compared with the 11u version, so I
didn't notice this in reviewing the patch itself.
Incidentally, we haven't been building JFR on x86 so far, because it
crashed when I tried to build it:
/builddir/build/BUILD/java-1.8.0-openjdk-1.8.0.262.b02-0.2.ea.el7.i386/openjdk/build/jdk8.build-debug/images/j2sdk-image/bin/javac
-d . /builddir/build/SOURCES/TestCryptoLevel.java
# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc: SuppressErrorAt=/os.cpp:1281
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error
(/builddir/build/BUILD/java-1.8.0-openjdk-1.8.0.262.b02-0.2.ea.el7.i386/openjdk/hotspot/src/share/vm/runtime/os.cpp:1281),
pid=93916, tid=0xf6431b40
# assert(SerializePageShiftCount == count) failed: thread size changed,
fix SerializePageShiftCount constant
TestCryptoLevel.java is a simple Java test we run on the just-built JDK
to check unlimited crypto is in use:
https://src.fedoraproject.org/rpms/java-1.8.0-openjdk/blob/master/f/TestCryptoLevel.java
I assumed from this that x86 wasn't supported, but if it should be
working, maybe we should look into this more.
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
More information about the jdk8u-dev
mailing list