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

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

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

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 with a new target base due to a merge or a rebase. The pull request now contains five commits:

 - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
   
   reimplement this feature. withdraw my intrusive change in outputStream.
   use stringStream only for the constant OopPtr. after oop->print_on(st),
   delete all appearances of '\n'
 - Merge branch 'master' into JDK-8260198
 - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
   
   fix merge conflict.
 - Merge branch 'master' into JDK-8260198
 - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
   
   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 *

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

Changes: https://git.openjdk.java.net/jdk/pull/2178/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2178&range=03
  Stats: 51 lines in 4 files changed: 45 ins; 1 del; 5 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 [v4]

Evgeny Astigeevich
On Sun, 14 Feb 2021 05:44:51 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 with a new target base due to a merge or a rebase. The pull request now contains five commits:
>
>  - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>    
>    reimplement this feature. withdraw my intrusive change in outputStream.
>    use stringStream only for the constant OopPtr. after oop->print_on(st),
>    delete all appearances of '\n'
>  - Merge branch 'master' into JDK-8260198
>  - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>    
>    fix merge conflict.
>  - Merge branch 'master' into JDK-8260198
>  - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>    
>    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 *

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

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

> 4047:       ss.print(" ");
> 4048:       const_oop()->print_oop(&ss);
> 4049:       ss.tr_delete('\n');

`tr_delete` is expensive. Also deleting something in a stream does not fit into a concept of streams.
I see that the content of `ss` is traversed many times.
What about this code:
for (const char *str = ss.base(); *str; ) {
  size_t i = 0;
  while (str[i] && str[i] != '\n' ) {
       ++i;
  }
  st->print_raw(str, i);
  str += i;
  while (*str == '\n') {
    ++str;
  }
}

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

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 [v4]

Evgeny Astigeevich
On Wed, 17 Feb 2021 12:04:56 GMT, Evgeny Astigeevich <[hidden email]> wrote:

>> Xin Liu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
>>
>>  - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>>    
>>    reimplement this feature. withdraw my intrusive change in outputStream.
>>    use stringStream only for the constant OopPtr. after oop->print_on(st),
>>    delete all appearances of '\n'
>>  - Merge branch 'master' into JDK-8260198
>>  - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>>    
>>    fix merge conflict.
>>  - Merge branch 'master' into JDK-8260198
>>  - 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>>    
>>    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 *
>
> src/hotspot/share/opto/type.cpp line 4049:
>
>> 4047:       ss.print(" ");
>> 4048:       const_oop()->print_oop(&ss);
>> 4049:       ss.tr_delete('\n');
>
> `tr_delete` is expensive. Also deleting something in a stream does not fit into a concept of streams.
> I see that the content of `ss` is traversed many times.
> What about this code:
> for (const char *str = ss.base(); *str; ) {
>   size_t i = 0;
>   while (str[i] && str[i] != '\n' ) {
>        ++i;
>   }
>   st->print_raw(str, i);
>   str += i;
>   while (*str == '\n') {
>     ++str;
>   }
> }

Another option:
class filterStringStream: public stringStream {
private:
  char ch;
public:
  filterStringStream(char ch_to_filter, size_t initial_bufsize = 256) : stringStream(initial_bufsize), ch(ch_to_filter) {}

  virtual void write(const char* c, size_t len) override {
    const char* e = c + len;
    while (c != e) {
      size_t i = 0;
      while ((c+i) != e && c[i] != ch ) {
        ++i;
      }
      stringStream::write(c, i);
      c += i;
      while (c != e && *ch == ch) {
        ++c;
      }
    }    
  }
};

Your code will be:
filterStringStream ss('\n');
ss.print(" ");
const_oop->print_oop(&ss);
st->print_raw(ss.base(), ss,size());

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

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 [v4]

Evgeny Astigeevich
On Wed, 17 Feb 2021 19:16:59 GMT, Evgeny Astigeevich <[hidden email]> wrote:

>> src/hotspot/share/opto/type.cpp line 4049:
>>
>>> 4047:       ss.print(" ");
>>> 4048:       const_oop()->print_oop(&ss);
>>> 4049:       ss.tr_delete('\n');
>>
>> `tr_delete` is expensive. Also deleting something in a stream does not fit into a concept of streams.
>> I see that the content of `ss` is traversed many times.
>> What about this code:
>> for (const char *str = ss.base(); *str; ) {
>>   size_t i = 0;
>>   while (str[i] && str[i] != '\n' ) {
>>        ++i;
>>   }
>>   st->print_raw(str, i);
>>   str += i;
>>   while (*str == '\n') {
>>     ++str;
>>   }
>> }
>
> Another option:
> class filterStringStream: public stringStream {
> private:
>   char ch;
> public:
>   filterStringStream(char ch_to_filter, size_t initial_bufsize = 256) : stringStream(initial_bufsize), ch(ch_to_filter) {}
>
>   virtual void write(const char* c, size_t len) override {
>     const char* e = c + len;
>     while (c != e) {
>       size_t i = 0;
>       while ((c+i) != e && c[i] != ch ) {
>         ++i;
>       }
>       stringStream::write(c, i);
>       c += i;
>       while (c != e && *ch == ch) {
>         ++c;
>       }
>     }    
>   }
> };
>
> Your code will be:
> filterStringStream ss('\n');
> ss.print(" ");
> const_oop->print_oop(&ss);
> st->print_raw(ss.base(), ss,size());

> `tr_delete` is expensive. Also deleting something in a stream does not fit into a concept of streams.
> I see that the content of `ss` is traversed many times.
> What about this code:
>
> ```
> for (const char *str = ss.base(); *str; ) {
>   size_t i = 0;
>   while (str[i] && str[i] != '\n' ) {
>        ++i;
>   }
>   st->print_raw(str, i);
>   str += i;
>   while (*str == '\n') {
>     ++str;
>   }
> }
> ```

You can put this code in a function like `print_filtering_ch(char, const stringStream&, outputStream*)`

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

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 [v4]

Xin Liu
On Wed, 17 Feb 2021 19:23:14 GMT, Evgeny Astigeevich <[hidden email]> wrote:

>> Another option:
>> class filterStringStream: public stringStream {
>> private:
>>   char ch;
>> public:
>>   filterStringStream(char ch_to_filter, size_t initial_bufsize = 256) : stringStream(initial_bufsize), ch(ch_to_filter) {}
>>
>>   virtual void write(const char* c, size_t len) override {
>>     const char* e = c + len;
>>     while (c != e) {
>>       size_t i = 0;
>>       while ((c+i) != e && c[i] != ch ) {
>>         ++i;
>>       }
>>       stringStream::write(c, i);
>>       c += i;
>>       while (c != e && *ch == ch) {
>>         ++c;
>>       }
>>     }    
>>   }
>> };
>>
>> Your code will be:
>> filterStringStream ss('\n');
>> ss.print(" ");
>> const_oop->print_oop(&ss);
>> st->print_raw(ss.base(), ss,size());
>
>> `tr_delete` is expensive. Also deleting something in a stream does not fit into a concept of streams.
>> I see that the content of `ss` is traversed many times.
>> What about this code:
>>
>> ```
>> for (const char *str = ss.base(); *str; ) {
>>   size_t i = 0;
>>   while (str[i] && str[i] != '\n' ) {
>>        ++i;
>>   }
>>   st->print_raw(str, i);
>>   str += i;
>>   while (*str == '\n') {
>>     ++str;
>>   }
>> }
>> ```
>
> You can put this code in a function like `print_filtering_ch(char, const stringStream&, outputStream*)`

hi, @eastig ,
 
Thank you for reviewing this code.

you are right, I shouldn't modify the contents of a stringStream. I treated it as a buffer instead of stream.
I took you advice, I moved `tr_delete` logic to StringUtils, which is a toolkit class.
Could you take a look again?

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

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