Re: RFR 8217647: [backport] JFR: recordings on 32-bit systems unreadable

[Denghui Dong]

Hi Andrey,

I haven't encountered this problem(I don't have any 32-bit environment).

For me, this patch is mainly for code unification, but I think this problem exists in 8u.

Denghui

------------------------------------------------------------------
From:Andrey Petushkov <andrey at azul.com>
Send Time:2020年6月16日(星期二) 18:11
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

Hi Denghui,

Just in case, do you actually observe any problems on 32-bit systems, or
it's just for a sake of unification of the code?

Andrey

On 16.06.2020 06: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