Thanks David! I'm waiting for second reviewer.
Yasumasa 2018-06-14 15:52 GMT+09:00 David Holmes <david.hol...@oracle.com>: > Seems fine. > > Thanks, > David > > > On 14/06/2018 3:01 PM, Yasumasa Suenaga wrote: >> >> Hi David, >> >> Thank you for your comment. >> >> I removed the assertion from PerfDataEntry.java . >> Could you review again? >> >> http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.03/ >> >> >> Thanks, >> >> Yasuamsa >> >> >> >> 2018-06-14 13:41 GMT+09:00 David Holmes <david.hol...@oracle.com>: >>> >>> Hi Yasumasa, >>> >>> This seems mostly okay - though like Thomas this is not an area I'm that >>> familiar with. But it seems this addresses the problem without impacting >>> other things. >>> >>> This assert is redundant: >>> >>> 366 if (Assert.ASSERTS_ENABLED) { >>> 367 Assert.that(vectorLength() > 0, "not a byte >>> vector"); >>> >>> You can't reach it unless len != 0 and I assume you're not really >>> checking >>> that len is not-negative - if you are then the assert needs to go further >>> up >>> after: >>> >>> 328 int len = vectorLength(); >>> >>> Thanks, >>> David >>> >>> >>> On 14/06/2018 2:18 PM, Yasumasa Suenaga wrote: >>>> >>>> >>>> Hi Thomas, >>>> >>>> Thank you for your comment. >>>> I uploaded new webrev: >>>> >>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.02/ >>>> >>>> >>>> 2018-06-14 3:34 GMT+09:00 Thomas Stüfe <thomas.stu...@gmail.com>: >>>>> >>>>> >>>>> Hi Yasumasa, >>>>> >>>>> looks good to me. >>>>> >>>>> small nit: >>>>> import java.nio.charset.* -> import java.nio.charset.Charset; >>>> >>>> >>>> >>>> Fixed. >>>> >>>> >>>>> We loose the assert for type now from byteArrayValue(). Can you re-add >>>>> it to the location where you call CStringUtils? >>>> >>>> >>>> >>>> I added the assertion to check vector length. >>>> I did not add type assertion because if-statement branches by data type. >>>> >>>> >>>>> We do not seem to need byteArrayValue(), can this be removed? >>>> >>>> >>>> >>>> I cannot agree that. >>>> byteArrayValue() is public method (but it is not used AFAICS). >>>> However, similar getter methods are privided from PerfDataEntry (e.g. >>>> charArrayValue()). So I think we should not remove byteArrayValue(). >>>> >>>> >>>> Regards, >>>> >>>> Yasumasa >>>> >>>> >>>>> Best Regards, Thomas >>>>> >>>>> >>>>> >>>>> On Wed, Jun 13, 2018 at 3:33 PM, Yasumasa Suenaga <yasue...@gmail.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> Thanks Thomas, >>>>>> >>>>>>> That said, I'd probably just add a variant to >>>>>>> CStringUtilities::getString which takes encoding as an argument, and >>>>>>> then pass "US-ASCII" to remain compatible with the current version of >>>>>>> jsnap. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I agree with you. My opinion is one of the ideals. >>>>>> Anyway, I uploaded new webrev which is added charset support in >>>>>> CStringUtils: >>>>>> >>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.01/ >>>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Yasumasa >>>>>> >>>>>> >>>>>> >>>>>> On 2018/06/13 21:46, Thomas Stüfe wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Disclaimer: I am Reviewer but not versed in this corner of the jdk, >>>>>>> nor have knowledge of the history, so take what I suggest with a >>>>>>> grain >>>>>>> of salt. >>>>>>> >>>>>>> That said, I'd probably just add a variant to >>>>>>> CStringUtilities::getString which takes encoding as an argument, and >>>>>>> then pass "US-ASCII" to remain compatible with the current version of >>>>>>> jsnap. >>>>>>> >>>>>>> ..Thomas >>>>>>> >>>>>>> On Wed, Jun 13, 2018 at 2:37 PM, Yasumasa Suenaga >>>>>>> <yasue...@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Thomas, >>>>>>>> >>>>>>>> Thank you for your comment. >>>>>>>> >>>>>>>> CStringUtilities.java says the String which is constructed from byte >>>>>>>> array >>>>>>>> should use Charset.defaultCharset(). [1] >>>>>>>> I'm not convinced how should we fix - US-ASCII or default charset. >>>>>>>> >>>>>>>> IMHO we should use default charset. However I concern I need to get >>>>>>>> CSR >>>>>>>> approvement. >>>>>>>> (In addition, I want to refactor CStringUtilities. I guess we can >>>>>>>> make >>>>>>>> more >>>>>>>> simple code :-) >>>>>>>> >>>>>>>> >>>>>>>> What do you think about it? >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>>>> >>>>>>>> [1] >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/7bf4f1b5e438/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/CStringUtilities.java#l73 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 2018/06/13 21:18, Thomas Stüfe wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Yasumasa, >>>>>>>>> >>>>>>>>> your patch will change behavior of jsnap if file.encoding is set to >>>>>>>>> something different than US-ASCII. >>>>>>>>> >>>>>>>>> Old version uses, hard wired, US-ASCII. >>>>>>>>> >>>>>>>>> You now use jvm/hotspot/utilities/CStringUtilities.java, which uses >>>>>>>>> >>>>>>>>> private static String encoding = >>>>>>>>> System.getProperty("file.encoding", >>>>>>>>> "US-ASCII"); >>>>>>>>> >>>>>>>>> and thus may use a different encoding. >>>>>>>>> >>>>>>>>> Best Regards, Thomas >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Jun 13, 2018 at 2:09 PM, Yasumasa Suenaga >>>>>>>>> <yasue...@gmail.com> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> PING: Could you review it? >>>>>>>>>> I want to merge this change before JDK 11 RDP 1. >>>>>>>>>> >>>>>>>>>>>>> webrev: >>>>>>>>>>>>> >>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.00/ >>>>>>>>>>>>> >>>>>>>>>>>>> JBS: >>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204531 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yasumasa >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 2018/06/08 15:38, Yasumasa Suenaga wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Hi David, >>>>>>>>>>> >>>>>>>>>>> 2018-06-08 10:47 GMT+09:00 David Holmes >>>>>>>>>>> <david.hol...@oracle.com>: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi Yasumasa, >>>>>>>>>>>> >>>>>>>>>>>> On 7/06/2018 10:43 PM, Yasumasa Suenaga wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Hi all, >>>>>>>>>>>>> >>>>>>>>>>>>> Please review this change: >>>>>>>>>>>>> >>>>>>>>>>>>> webrev: >>>>>>>>>>>>> >>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.00/ >>>>>>>>>>>>> >>>>>>>>>>>>> JBS: >>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204531 >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> We can use `jhsdb jsnap` to check all PerfData. >>>>>>>>>>>>> String values in PerfData are defined as jbyte array, but JSnap >>>>>>>>>>>>> cannot >>>>>>>>>>>>> handle it well as following: >>>>>>>>>>>>> >>>>>>>>>>>>> ``` >>>>>>>>>>>>> $ jhsdb jsnap --pid 28542 --all | less >>>>>>>>>>>>> >>>>>>>>>>>>> sun.gc.cause=No >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> GC^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@ >>>>>>>>>>>>> ``` >>>>>>>>>>>>> >>>>>>>>>>>>> You can see this value via `less` and `vim` on Linux. `^@` >>>>>>>>>>>>> shows >>>>>>>>>>>>> it >>>>>>>>>>>>> is >>>>>>>>>>>>> non-ascii character. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> So your synopsis says "remove unused chars following \0". Is >>>>>>>>>>>> that >>>>>>>>>>>> really >>>>>>>>>>>> what is happening? I would have expected "new String(bytes, >>>>>>>>>>>> encoding)" >>>>>>>>>>>> to >>>>>>>>>>>> stop processing bytes when it encounters a NUL! >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> String c'tor will continue to parse byte array even if it finds >>>>>>>>>>> '\0'. >>>>>>>>>>> You can see it on JShell as below: >>>>>>>>>>> >>>>>>>>>>> ------------- >>>>>>>>>>> jshell> var bytes = new byte[]{(byte)'a', (byte)'b', (byte)'c', >>>>>>>>>>> (byte)'\0', (byte)'d', (byte)'e', (byte)'f'} >>>>>>>>>>> bytes ==> byte[7] { 97, 98, 99, 0, 100, 101, 102 } >>>>>>>>>>> >>>>>>>>>>> jshell> new String(bytes, "US-ASCII") >>>>>>>>>>> $2 ==> "abc\000def" >>>>>>>>>>> ------------- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>>> PerfDataEntry has null-terminated C string. So we should >>>>>>>>>>>>> restore >>>>>>>>>>>>> as >>>>>>>>>>>>> it >>>>>>>>>>>>> in >>>>>>>>>>>>> Java layer. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> If the issue is really "junk" beyond the \0 nul-terminator, how >>>>>>>>>>>> did >>>>>>>>>>>> we >>>>>>>>>>>> get >>>>>>>>>>>> such values? Shouldn't the byte array already be terminated at >>>>>>>>>>>> the >>>>>>>>>>>> NUL? >>>>>>>>>>>> Or >>>>>>>>>>>> is this just a raw representation of an array in the VM using >>>>>>>>>>>> the >>>>>>>>>>>> exact >>>>>>>>>>>> length of the array regardless of content? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I think JSnap shows the array regardless of content. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> I see CStringUtilities.getString() both stops when it encounters >>>>>>>>>>>> a >>>>>>>>>>>> 0 >>>>>>>>>>>> value, >>>>>>>>>>>> and uses the default file encoding (else ASCII). >>>>>>>>>>>> >>>>>>>>>>>> The fix seems perfectly reasonable, I'm just unclear where >>>>>>>>>>>> exactly >>>>>>>>>>>> this >>>>>>>>>>>> "bad" data should have been stopped. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks! >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yasumasa >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> David >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> >>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>> >
