PING: RFR: 8204531: Remove unused chars following '\0'
David Holmes
david.holmes at oracle.com
Thu Jun 14 06:52:24 UTC 2018
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.holmes at 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.stuefe at 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 <yasuenag at 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 <yasuenag at 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
>>>>>>>> <yasuenag at 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.holmes at 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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>
More information about the serviceability-dev
mailing list