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

[Leonid Mesnik]

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


-- 
Leonid Mesnik
JVM SQE