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 >>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>>> >
