Thanks Thomas!
Yasumasa 2018-06-14 16:45 GMT+09:00 Thomas Stüfe <thomas.stu...@gmail.com>: > Hi Yasumasa, > > this is fine for me. Thanks for fixing! > > ..Thomas > > On Thu, Jun 14, 2018 at 9:37 AM, Yasumasa Suenaga <yasue...@gmail.com> wrote: >> 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 >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>> >>>
