RFR: 8009763 Add WB test for String.intern()

[Leonid Mesnik]

Thank you very match for sponsoring push.

Leonid
On 04/02/2013 01:18 PM, Mikael Gerdin wrote:
> Leonid,
>
> On 2013-03-29 15:02, Leonid Mesnik wrote:
>> Zhengyu
>>
>> Thank you very match for review. I've removed 'unsigned', updated
>> indentation and check with JPRT.
>>
>> Here is new webrev:
>> http://cr.openjdk.java.net/~mgerdin/lmesnik/webrev.3/
>> <http://cr.openjdk.java.net/%7Emgerdin/lmesnik/webrev.3/>
>
> This looks good to me. There is some strange indentaion in 
> SanityTest.java but I can fix that myself when I sponsor your push.
>
> /Mikael
>
>>
>>
>> Leonid
>>
>> On 03/27/2013 11:49 PM, Zhengyu Gu wrote:
>>> Hi Leonid,
>>>
>>> symbolTable.cpp #687
>>>   Why converts to "unsigned int"?  it appears that hash_to_index()
>>> returns "int", and the_table()->lookup() takes "int"
>>>
>>> Other than that, good to me.
>>>
>>> -Zhengyu
>>>
>>> On 3/22/2013 3:07 AM, Leonid Mesnik wrote:
>>>> Are there any other comments?
>>>>
>>>> Leonid
>>>> On 03/20/2013 03:41 PM, Leonid Mesnik wrote:
>>>>> Hi
>>>>>
>>>>> Here is next attempt:
>>>>> http://cr.openjdk.java.net/~mgerdin/lmesnik/webrev.2/
>>>>> <http://cr.openjdk.java.net/%7Emgerdin/lmesnik/webrev.2/>
>>>>>
>>>>> The StringTable::lookup methods were updated.
>>>>>
>>>>> Leonid
>>>>> On 03/14/2013 06:38 PM, Mikael Gerdin wrote:
>>>>>> Lenoid,
>>>>>>
>>>>>> On 2013-03-13 11:14, Leonid Mesnik wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> werbrev was updated (thank you for this)
>>>>>>> http://cr.openjdk.java.net/~mgerdin/lmesnik/webrev.1/
>>>>>>> <http://cr.openjdk.java.net/%7Emgerdin/lmesnik/webrev.1/>
>>>>>>
>>>>>> Also, I don't know why you have "StringTabe::" inside these calls:
>>>>>>
>>>>>>    unsigned int hash = StringTable::hash_string(name, len);
>>>>>>    unsigned int index = 
>>>>>> StringTable::the_table()->hash_to_index(hash);
>>>>>>    return StringTable::the_table()->lookup(index, name, len, hash);
>>>>>>
>>>>>> Since you are in an instance method in StringTable you also
>>>>>> shouldn't get the_table() singleton, but rather use the current
>>>>>> instance.
>>>>>>
>>>>>>
>>>>>> I think your new lookup() function could be static just as the
>>>>>> other public lookup() function.
>>>>>>
>>>>>> Another idea is to perhaps make these two lookup:s share the same
>>>>>> implementation:
>>>>>>
>>>>>> oop StringTable::lookup(Symbol* symbol) {
>>>>>>    ResourceMark rm;
>>>>>>    int length;
>>>>>>    jchar* chars = symbol->as_unicode(length);
>>>>>>    return lookup(chars, length);
>>>>>> }
>>>>>>
>>>>>> oop StringTable::lookup(const jchar* chars, int length) {
>>>>>>    unsigned int hashValue = hash_string(chars, length);
>>>>>>    int index = the_table()->hash_to_index(hashValue);
>>>>>>    return the_table()->lookup(index, chars, length, hashValue);
>>>>>> }
>>>>>>
>>>>>> /Mikael
>>>>>>
>>>>>>>
>>>>>>> JPRT passed with -rtest '*-*-*-runtime/interned'
>>>>>>>
>>>>>>> Leonid
>>>>>>> On 03/12/2013 05:05 PM, Mikael Gerdin wrote:
>>>>>>>> Hi Leonid,
>>>>>>>>
>>>>>>>> On 2013-03-12 08:25, Leonid Mesnik wrote:
>>>>>>>>> Hi
>>>>>>>>> I've added a small test which:
>>>>>>>>> 1) creates java string and intern it
>>>>>>>>> 2) verify that it presents in StringTable
>>>>>>>>> 3) clear reference and call fullGC
>>>>>>>>> 4) verify that there is no such string in StringTable
>>>>>>>>>
>>>>>>>>> Here is webrev:
>>>>>>>>> http://cr.openjdk.java.net/~mgerdin/lmesnik/webrev.0/
>>>>>>>>
>>>>>>>> Overall this looks reasonable.
>>>>>>>>
>>>>>>>> I have some comments though:
>>>>>>>> WB_IsInStringTable is defined to return a jboolean
>>>>>>>> (Ljava/lang/String;)Z
>>>>>>>> but the actual function is defined to return an jint. This will
>>>>>>>> probably work but you should fix this to make it consistent.
>>>>>>>>
>>>>>>>> The function WB_fullGC should have a capital F to be consistent 
>>>>>>>> with
>>>>>>>> the naming of the other WB_* functions.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Tested locally and by JPRT
>>>>>>>>>
>>>>>>>> did you run jprt with -retest '*-*-*-runtime/interned' as well?
>>>>>>>>
>>>>>>>> /Mikael
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>


-- 
Leonid Mesnik
JVM SQE