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

[Zhengyu Gu]

Look good.

-Zhengyu

On 3/29/2013 10:02 AM, 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/>
>
>
> 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
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>
>