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

[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
 
  fix build failures on Windows. StringUtils::tr_delete returns size_of.

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

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

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

  Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 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

[Evgeny Astigeevich]

On Sun, 21 Feb 2021 02:07:59 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
>  
>   fix build failures on Windows. StringUtils::tr_delete returns size_of.

Changes requested by [hidden email] (no known OpenJDK username).

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

> 4054:         StringUtils::tr_delete(buf, "\n");
> 4055:         st->print_raw(buf);
> 4056:         os::free(buf);

There is no need to use `os::strdup` because `as_string` creates a copy.
I've looked in stringUtils.cpp and found `replace_no_expand`.  The code can be rewritten:
char *buf = ss.as_string();
StringUtils::replace_no_expand(buf, "\n", "");
st->print_raw(buf);
With this code, `tr_delete` is redundant.

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

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

[Xin Liu]

On Tue, 23 Feb 2021 15:59:03 GMT, Evgeny Astigeevich <[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
>>  
>>   fix build failures on Windows. StringUtils::tr_delete returns size_of.
>
> src/hotspot/share/opto/type.cpp line 4056:
>
>> 4054:         StringUtils::tr_delete(buf, "\n");
>> 4055:         st->print_raw(buf);
>> 4056:         os::free(buf);
>
> There is no need to use `os::strdup` because `as_string` creates a copy.
> I've looked in stringUtils.cpp and found `replace_no_expand`.  The code can be rewritten:
> char *buf = ss.as_string();
> StringUtils::replace_no_expand(buf, "\n", "");
> st->print_raw(buf);
> With this code, `tr_delete` is redundant.

oh, thanks for the head-up.  I'm happy to remove os::strdup and os::free pair.
it seems that replace_no_expand is cumbersome to do what  tr_delete does.  let me see how it works.

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

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

[Evgeny Astigeevich]

On Tue, 23 Feb 2021 19:30:00 GMT, Xin Liu <[hidden email]> wrote:

> oh, thanks for the head-up. I'm happy to remove os::strdup and os::free pair.
> it seems that replace_no_expand is cumbersome to do what tr_delete does. let me see how it works.

I don't see why it is cumbersome. IMHO, it is logically consistent: replace substring with an empty string without expanding the buffer. The main value is the amount of written code.

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

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

[Xin Liu]

On Tue, 23 Feb 2021 21:09:15 GMT, Evgeny Astigeevich <[hidden email]> wrote:

>> oh, thanks for the head-up.  I'm happy to remove os::strdup and os::free pair.
>> it seems that replace_no_expand is cumbersome to do what  tr_delete does.  let me see how it works.
>
>> oh, thanks for the head-up. I'm happy to remove os::strdup and os::free pair.
>> it seems that replace_no_expand is cumbersome to do what tr_delete does. let me see how it works.
>
> I don't see why it is cumbersome. IMHO, it is logically consistent: replace substring with an empty string without expanding the buffer. The main value is the amount of written code.

oh, by means "cumbersome",  I just felt that it's easier to sweeping chars than substrings in my case.  but I has verified `replace_no_expand(buf, "\n", "")` has the same effect.  I took you advice.  less code is less chance to make mistake.

Updated this PR. I also verified that it has the same results for `-XX:+Verbose -XX:+PrintIdeal`.

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

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