Re: RFR: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set [v7]

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set [v7]

Xin Liu
> Add a flag _suppress_cr to outputStream. outstream objects won't emit any CR if it's set.
> Correct TypeInstPtr::dump2 to make sure it only emits klass name once.
> Remove the comment because Klass::oop_print_on() has emitted the address of oop.
>
> Before:
> 689  ConP  ===  0  [[ 821 ]]   Oop:java/lang/Stringjava.lang.String
> {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String'
> - string: "a"
> :Constant:exact *
>
> After:
> 689  ConP  ===  0  [[ 821 ]]   Oop:java.lang.String {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String' - string: "a":Constant:exact *

Xin Liu has updated the pull request incrementally with one additional commit since the last revision:

  8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
 
  use the existing api StringUtils::replace_no_expand to archive the same replace.
  don't need to invoke os::strdup because stringStream::as_string() has duplicated
  the internal buffer.

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2178/files
  - new: https://git.openjdk.java.net/jdk/pull/2178/files/077f9b60..6df63fe1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2178&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2178&range=05-06

  Stats: 83 lines in 5 files changed: 0 ins; 75 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2178.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2178/head:pull/2178

PR: https://git.openjdk.java.net/jdk/pull/2178
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set [v7]

Tobias Hartmann-3
On Tue, 23 Feb 2021 23:12:56 GMT, Xin Liu <[hidden email]> wrote:

>> Add a flag _suppress_cr to outputStream. outstream objects won't emit any CR if it's set.
>> Correct TypeInstPtr::dump2 to make sure it only emits klass name once.
>> Remove the comment because Klass::oop_print_on() has emitted the address of oop.
>>
>> Before:
>> 689  ConP  ===  0  [[ 821 ]]   Oop:java/lang/Stringjava.lang.String
>> {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String'
>> - string: "a"
>> :Constant:exact *
>>
>> After:
>> 689  ConP  ===  0  [[ 821 ]]   Oop:java.lang.String {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String' - string: "a":Constant:exact *
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
>
>   8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>  
>   use the existing api StringUtils::replace_no_expand to archive the same replace.
>   don't need to invoke os::strdup because stringStream::as_string() has duplicated
>   the internal buffer.

Changes requested by thartmann (Reviewer).

src/hotspot/share/opto/type.cpp line 4052:

> 4050:
> 4051:       {
> 4052:         ResourceMark rm;

Shouldn't the `ResourceMark` go to before `stringStream ss` which is a `ResourceObj` as well? Also, please add a small comment explaining that this code suppresses the new line emitted by `print_oop`.

src/hotspot/share/opto/type.cpp line 4040:

> 4038: // Dump oop Type
> 4039: #ifndef PRODUCT
> 4040: void TypeInstPtr::dump2( Dict &d, uint depth, outputStream* st ) const {

While you are at it, please also remove the excess whitespace after `(` and before `)`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2178
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set [v7]

Xin Liu
On Wed, 24 Feb 2021 06:50:51 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>>  
>>   use the existing api StringUtils::replace_no_expand to archive the same replace.
>>   don't need to invoke os::strdup because stringStream::as_string() has duplicated
>>   the internal buffer.
>
> src/hotspot/share/opto/type.cpp line 4052:
>
>> 4050:
>> 4051:       {
>> 4052:         ResourceMark rm;
>
> Shouldn't the `ResourceMark` go to before `stringStream ss` which is a `ResourceObj` as well? Also, please add a small comment explaining that this code suppresses the new line emitted by `print_oop`.

hi, @TobiHartmann
Thank you for reviewing this PR.

stringStream allocates its dynamic buffer using `NEW_C_HEAP_ARRAY`.  IMHO, it's okay without a ResourceMark.
Unlike `stringStream::grow`, stringStream::as_string(false) does use `NEW_RESOURCE_ARRAY`, which allocates an array on current thread's resource_area. That's why I put ResourceMark in a syntax scope.

Actually, the current code still works even without that ResourceMark. It's because `Type::dump_on()` has  declared a rm. Let me hoist ResourceMark as you said. That makes code straight-forward locally and I shouldn't assume its context.

> src/hotspot/share/opto/type.cpp line 4040:
>
>> 4038: // Dump oop Type
>> 4039: #ifndef PRODUCT
>> 4040: void TypeInstPtr::dump2( Dict &d, uint depth, outputStream* st ) const {
>
> While you are at it, please also remove the excess whitespace after `(` and before `)`.

done.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2178