Quantcast

RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

Aleksey Shipilev-4
https://bugs.openjdk.java.net/browse/JDK-8180482

Current table is garbled because VM ops names are too long, headers are not
accounted in format specifiers, etc.

Patch:
  http://cr.openjdk.java.net/~shade/8180482/webrev.01/

Before/after:
  http://cr.openjdk.java.net/~shade/8180482/before.txt
  http://cr.openjdk.java.net/~shade/8180482/after.txt

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

David Holmes
Hi Aleksey,

On 17/05/2017 9:02 PM, Aleksey Shipilev wrote:
> https://bugs.openjdk.java.net/browse/JDK-8180482
>
> Current table is garbled because VM ops names are too long, headers are not
> accounted in format specifiers, etc.
>
> Patch:
>   http://cr.openjdk.java.net/~shade/8180482/webrev.01/

Can't say I like all the magic numbers. Is this:

   tty->print("[ %5s %7s %7s %7s %7s %7s ] ",
              "time:", "spin", "block", "sync", "cleanup", "vmop");

really worth the effort versus:

    // widest column name needs 7 chars so space accordingly
   tty->print("[ time:    spin   block    sync cleanup    vmop ]");

?

There's nothing to link the width specifiers in the header with those
used in the print function.

David

> Before/after:
>   http://cr.openjdk.java.net/~shade/8180482/before.txt
>   http://cr.openjdk.java.net/~shade/8180482/after.txt
>
> Thanks,
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

Aleksey Shipilev-4
Hi David,

On 05/17/2017 02:19 PM, David Holmes wrote:

> On 17/05/2017 9:02 PM, Aleksey Shipilev wrote:
>> Patch:
>>   http://cr.openjdk.java.net/~shade/8180482/webrev.01/
>
> Can't say I like all the magic numbers. Is this:
>
>   tty->print("[ %5s %7s %7s %7s %7s %7s ] ",
>              "time:", "spin", "block", "sync", "cleanup", "vmop");
>
> really worth the effort versus:
>
>    // widest column name needs 7 chars so space accordingly
>   tty->print("[ time:    spin   block    sync cleanup    vmop ]");
>
> ?

Agreed, it was easier to handle with width specifiers when tidying up the code.
The final version can just use the appropriate number of spaces:
  http://cr.openjdk.java.net/~shade/8180482/webrev.02/

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

David Holmes
On 17/05/2017 11:24 PM, Aleksey Shipilev wrote:

> Hi David,
>
> On 05/17/2017 02:19 PM, David Holmes wrote:
>> On 17/05/2017 9:02 PM, Aleksey Shipilev wrote:
>>> Patch:
>>>   http://cr.openjdk.java.net/~shade/8180482/webrev.01/
>>
>> Can't say I like all the magic numbers. Is this:
>>
>>   tty->print("[ %5s %7s %7s %7s %7s %7s ] ",
>>              "time:", "spin", "block", "sync", "cleanup", "vmop");
>>
>> really worth the effort versus:
>>
>>    // widest column name needs 7 chars so space accordingly
>>   tty->print("[ time:    spin   block    sync cleanup    vmop ]");
>>
>> ?
>
> Agreed, it was easier to handle with width specifiers when tidying up the code.
> The final version can just use the appropriate number of spaces:
>   http://cr.openjdk.java.net/~shade/8180482/webrev.02/

Looks much simpler :)

This is also overkill for adding spaces:

1269     tty->print("[ %5s "

but I'd take it as-is as well. Don't forget to update copyright year
before pushing.

I think this qualifies as a trivial change (only one Reviewer needed).

Thanks,
David

> Thanks,
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

Aleksey Shipilev-4
On 05/17/2017 11:09 PM, David Holmes wrote:
> This is also overkill for adding spaces:
>
> 1269     tty->print("[ %5s "

Oh yeah, fixed.

> but I'd take it as-is as well. Don't forget to update copyright year before
> pushing.

Right.

> I think this qualifies as a trivial change (only one Reviewer needed).

I don't think I can push myself, JPRT and all that?

Please sponsor:
  http://cr.openjdk.java.net/~shade/8180482/8180482.changeset

-Aleksey



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8180482: Reformat -XX:+PrintSafepointStatistics table

David Holmes
On 18/05/2017 7:20 AM, Aleksey Shipilev wrote:

> On 05/17/2017 11:09 PM, David Holmes wrote:
>> This is also overkill for adding spaces:
>>
>> 1269     tty->print("[ %5s "
>
> Oh yeah, fixed.
>
>> but I'd take it as-is as well. Don't forget to update copyright year before
>> pushing.
>
> Right.
>
>> I think this qualifies as a trivial change (only one Reviewer needed).
>
> I don't think I can push myself, JPRT and all that?

Oops - forgot :)

> Please sponsor:
>   http://cr.openjdk.java.net/~shade/8180482/8180482.changeset

Will do.

David

> -Aleksey
>
>
>
Loading...