[11u] RFR: JDK-8205516: JFR tool

[Severin Gehwolf]

Hi Jie,

On Mon, 2019-08-12 at 14:58 -0400, Jie Kang wrote:
> Hi all,
> 
> Please review this jdk11u backport. The patch did *not* apply cleanly
> and has changes described following the links below.  A CSR was opened
> and approved for this backport. As well Oracle has brought this for
> 11.0.6 so we should not push this until 11.0.6 opens (Aug 27).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8205516
> Webrev: http://cr.openjdk.java.net/~jkang/8205516/webrev.01/

src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java

JDK 11u has JDK-8215175 already applied which removes this function:
public static String formatBytes(long bytes, String separation)

Thus, changes to Utils.java don't apply for JDK 11u. OK.

src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java
src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordedObject.java

Changes from JDK-8165675 incorporated as you say. OK

src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java

You seem to have introduced an extra newline at line 314. Please remove
as this might cause issues on further backports.

517                 String bytes = Utils.formatBytes(n.longValue());

Changed 'Utils.formatBytes(n.longValue(), " ")' to
'Utils.formatBytes(n.longValue())' so as to account for JDK-8215175
already been present in JDK 11u as you said. OK.

test/jdk/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java

Test adjusted back to post JDK-8215175 changes in JDK 11u. OK.

Thanks,
Severin


> CSR: https://bugs.openjdk.java.net/browse/JDK-8228670
> 
> * Addition of changes to PrettyWriter and RecordedObject from JDK-8165675 [1]
> 
> PrettyWriter is newly added with this backport. Since it did not exist
> at the time for the JDK-8165675 backport, the changes there for
> PrettyWriter were not included. These are included within this patch.
> As well some lines in the JDK-8165675 backport were skipped for
> RecordedObject causing a test to fail, and are now included so the
> test passes. Let me know if there is a different process for this
> situation!
> 
> [1]
> https://bugs.openjdk.java.net/browse/JDK-8165675
> PrettyWriter changes:
> http://hg.openjdk.java.net/jdk/jdk/rev/4eff16f47ae2#l5.2
> RecordedObject changes:
> http://hg.openjdk.java.net/jdk/jdk/rev/4eff16f47ae2#l4.35
> 
> * Updates to test/jdk/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java
> 
> This test is updated to match this backport. There are comments in the
> changeset visible that describe why.
> 
> * A small update to
> src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java
> 
>          if (dataAmount != null) {
>              if (value instanceof Number) {
>                  Number n = (Number) value;
> -                String bytes = Utils.formatBytes(n.longValue(), " ");
> +                String bytes = Utils.formatBytes(n.longValue());
>                  if (field.getAnnotation(Frequency.class) != null) {
>                      bytes += "/s";
>                  }
> 
> This is to match the function definition of Utils.formatBytes in
> jdk11u-dev. This is actually also what is in jdk/jdk, but it is a
> change here as the order of commits in jdk11u-dev differ from jdk/jdk.
> 
> 
> Also. this backport should be integrated with the two follow-up bug
> fixes at the same time to maintain a clean working repository
> (compiles and tests pass). Both cleanly apply after my patch. I have
> added fix request label and comment to them.
> 
> JDK-8214896: JFR Tool left files behind
> https://bugs.openjdk.java.net/browse/JDK-8214896
> 
> JDK-8214925: [TESTBUG] JFR tool fails to execute
> https://bugs.openjdk.java.net/browse/JDK-8214925
> 
> I have run the tier one tests, jdk.jfr tests and some manual tests of
> the jfr tool binary with all three fixes applied together, but third
> party verification would be greatly appreciated. Let me know what you
> think!
> 
> 
> Regards,