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