[PATCH for preview] Add version and effective JVM path to the JSON output

Aleksey Shipilev ashipile at redhat.com
Mon Apr 17 09:39:07 UTC 2017


Hi Jens,

Sorry for late reply: conference/vacation weeks.

On 04/03/2017 05:46 AM, Jens Wilke wrote:
> I got spurious errors about wrong JavaDoc. That's why some JavaDoc fixes are sprinkled in, sorry

No worries, can be done within this patch.

> JVM version is included in the JSON file with the same print format we currently have. Example: "jvmVersion" : "JDK 1.8.0_121, VM 25.121-b13". 
> This is bad. We should have different fields for it and name it java.version and java.vm.version. To have the distinct values I tend to 
> change VersionMain to return the complete system properties in properties file format. Thoughts?

Yes, makes sense. VersionMain is internal anyway, and we can do whatever pleases
us with it.

> To have arbitrary strings in JSON is a major change. At the moment JMH is doing some tidying and pretty printing of the JSON via regular expressions.
> This fails for arbitrary string content. That might prove a bad trap, when there is the wrong character in a parameter. Options: Replace tidying maybe 
> even writing with a library; low tech: escape all special characters in strings, tidy, then replace back. I already added commons lang3 as 
> dependency to have JSON string escaping. Thoughts?

"Low tech" is my preference here.


Two comments for the patch:

> +        <dependency>
> +            <groupId>org.apache.commons</groupId>
> +            <artifactId>commons-lang3</artifactId>
> +            <version>3.5</version>
> +        </dependency>

We prefer not to have external dependencies. We now reference commons-math, and
that is already questionable...

> +            pw.println("\"jvm\" : \"" + StringEscapeUtils.escapeJson(params.getJvm().replace(',', ';')) + "\",");
> +            // if empty, write an empty array.
> +            pw.println("\"jvmArgs\" : [");
> +            printStringArray(pw, params.getJvmArgs());
> +            pw.println("],");
> +            // FIXME: the string contains JDK and VM versions, should be separate JSON field
> +            pw.println("\"jvmVersion\" : \"" + StringEscapeUtils.escapeJson(params.getJvmVersion().replace(',', ';')) + "\",");

I agree, better to split.

Thanks,
-Aleksey




More information about the jmh-dev mailing list