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

[Andrey Petushkov]

Ok. I fully do support idea of code unification.
Just one note on the problem, it definitely has existed early on. During
backporting to 8u all known to Azul problems have been fixed (in all
open code bases which are relevant to 8u). Later, these problems were
addressed in upstream separately by the community, and what you're
backporting is what, I believe, these fixes in upstream brought. The
approach of addressing the problems is different between the two, hence
you see the difference and merge conflicts.
So in my understanding 8u codebase does not need this fix per se, but as
I said, it's beneficial to have it for the purpose of code unification
and simplification of further backports

Andrey

On 16.06.2020 13:50, Denghui Dong wrote:
> 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
>