Please review 7178670: runtime/7158800/BadUtf8.java fails in SymbolTable::rehash_table (REVISED)

Coleen Phillimore coleen.phillimore at oracle.com
Mon Jun 25 07:16:23 PDT 2012


I found some bugs in stress testing the rehashing code (rehash_count = 
3, rehash_multiplier =1).   The code in symbolTable to basic_add cannot  
hit a safepoint because the table might be removed out from under it.  
I've moved the basic_add locking code to the static caller.

This also contains the review comments to date for CDS.

open webrev at http://cr.openjdk.java.net/~coleenp/hashmap_cds3/
(the 7u6 fixes are the same)

Thanks,
Coleen

On 6/22/2012 10:35 PM, Coleen Phillimore wrote:
>
>
> On 6/22/2012 2:13 PM, Daniel D. Daugherty wrote:
>> On 6/21/12 9:40 PM, Coleen Phillimore wrote:
>>>
>>> After some discussions about the preserving shared state in move_to, 
>>> it occurred to me that after rehashing, the shared entries were no 
>>> longer last in the buckets.   The walking in StringTable and 
>>> SymbolTable assumed this.    I also added more of a comment about 
>>> preserving the state.  It could be nicer code and safer but it would 
>>> also introduce some risk or possible performance impact.    These 
>>> are the latest webrevs which I've tested with rehash_count=2 and 
>>> rehash_multipier=1 to make it rehash a lot.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/hashmap_cds2
>>> open webrev at http://cr.openjdk.java.net/~coleenp/hashmap_7u6_cds2/
>>
>> src/share/vm/classfile/symbolTable.cpp
>>     line 102: Please consider the following rewrite of your comment:
>>
>>         // Shared entries are normally at the end of the table and if
>>         // we run into a shared entry, then there is nothing more to
>>         // remove. However, if we have rehashed the table, then nothing
>>         // is shared anymore.
>
> I like your comment, although it's not completely accurate.   The 
> entries are still shared in that they are still allocated in the 
> shared data misc section and cannot be removed.   So I used this comment.
>
>       // Shared entries are normally at the end of the bucket and if 
> we run into
>       // a shared entry, then there is nothing more to remove. 
> However, if we
>       // have rehashed the table, then the shared entries are no 
> longer at the
>       // end of the bucket.
>
>
>>
>>     line 143, 807: // This should never happen with -Xshare:dump but it
>>                       might in testing mode.
>>         These comments aren't comforting. Just curious: why might it 
>> happen
>>         in testing mode?
>
> If I set rehash_count to 1 to stress test this code, it'll rehash when 
> creating the shared archive, so I had to just ignore it when creating 
> the archive, since the archive cannot have an alternate hashing scheme 
> (don't save the seed).
>
>>
>>     line 723: should this be the following instead:
>>
>>         HashtableEntry<Symbol*>* entry =
>>           
>> (HashtableEntry<Symbol*>*)HashtableEntry<Symbol*>::make_ptr(*p);
>>
>>         That would match what you did on lines 99-100.
>
> Yes, I changed that.
>>
>> src/share/vm/utilities/hashtable.cpp
>>     line 123: // Keep the shared bit in the Hashtable entry so it 
>> can't be deleted.
>>
>>         If we've rehashed the table, then shared entries can "sort of"
>>         be deleted. They are deleted from the symbol table, but their
>>         memory is not freed. Am I understanding this right?
>>
>>         Maybe say "so it can't be freed". I don't know how to make this
>>         more clear.
>
> These entries are still not freed because the literal in the entry is 
> marked (Symbol is given refcount -1 and String oop is premarked) so it 
> cannot be freed.   The SymbolTable::unlink and String::unlink code 
> both rely on this as written.  I don't have to keep the shared bit 
> because if the table is rewritten, the shared bit doesn't provide a 
> quick exit to the loop in the unlink code.
>
> Coleen
>
>>
>>
>> src/share/vm/utilities/hashtable.hpp
>>     No comments.
>>
>>
>> Dan
>>
>>
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 6/21/2012 3:25 PM, Coleen Phillimore wrote:
>>>> Summary: Cannot delete _buckets and HashtableEntries in shared 
>>>> space (CDS)
>>>>
>>>> Tested with the tests that I added using Class Data Sharing (on and 
>>>> off).  This is both a patch for 7u6 and main.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/hashmap_cds/
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/hashmap_7u6_cds/
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>


More information about the hotspot-runtime-dev mailing list