Hi Ramki,
On 02/09/15 01:36, Srinivas Ramakrishna wrote:
Hi Bengt, Tony, Thomas -- [+serviceability-dev (see below)]
Thanks for your (re)reviews, and Bengt, thanks also for shepherding
the changes into the open jdk code. Much appreciated!!
(I'm hoping you can take care of the two white-space modifications
Thomas suggested and push the changes.)
I've fixed the whit-space changes. Thanks for pointing them out, Thomas!
Thomas, I am not quite sure of the value (or, utility in economic
terms) of something that checks for the format of the already somewhat
ill-defined (or undefined, ad-hoc) verbose +PrintReferenceGC gc
output. I'd just rely on the likes of Kirk and others who consume
these verbose log options to catch such occasional changes. (For
example, when Kim made the change for the processing of cleaners, she
probably knew of the change in format and figured it was fine -- I
would have probably felt the same. Although in the fullness of time,
perhaps it made sense to make the logging more fine-grained and with
more information.) Of course, I don't know if, with the new unified
logging work going on, there's an existing corpus of tests for JVM
generated logging where such a test might make sense. Are there such
tests? If so, could someone post a pointer? (I included the
serviceability alias for their possible input on the testing aspect of
this & possible pointers.) In general, there have been so many
changes to verbose gc log formatting (gc id's and such etc.) and the
flux of changes is so high that I think writing tests for these kinds
of things (which have no spec and change often at the whims of the
developers) have limited utility. Checking the correctness of the
<key, value>s emitted for various counters for something like JFR is
another matter and definitely worthwhile.
It's very easy to write a test that verifies the log entries. And I
agree with Thomas that it is worth while to have that test. No really as
a specification, but Just to make us aware when we do a change.
I've included the test in the updated webrev that I just sent out.
Thanks,
Bengt
thanks!
-- ramki
On Tue, Sep 1, 2015 at 10:02 AM, Thomas Schatzl
<thomas.scha...@oracle.com <mailto:thomas.scha...@oracle.com>> wrote:
On Tue, 2015-09-01 at 14:54 +0200, Bengt Rutisson wrote:
> Hi everyone,
>
> Could I have a couple of reviews for this patch contributed by
Ramki?
>
> http://cr.openjdk.java.net/~brutisso/8133818/webrev.00/
<http://cr.openjdk.java.net/%7Ebrutisso/8133818/webrev.00/>
> https://bugs.openjdk.java.net/browse/JDK-8133818
>
jvmtiTagMap.cpp, line 3408: the SIZE_FORMAT specifiers need a space
before and after, otherwise it will not compile with newer C++
compilers. Same for referenceProcessor.cpp, 307,
jvmtiTagMap.hpp, line 125: please fix the format of the method
parameter
specifications.
In general I would be really happy if there were a test checking the
output of -XX:+TraceReferenceGC. If we had had that earlier, this
issue
would have not cropped up, and prevents future issues.
I.e. just the format of "[whateverreference, <bb> refs, <cc> secs]
....
[PhantomReference, <xx> refs, <yyy> secs] ... [Cleaners, <zz> refs,
<aaa> secs] .."
Thanks,
Thomas
