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

[Coleen Phillimore]

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