Please review 7178670: runtime/7158800/BadUtf8.java fails in SymbolTable::rehash_table (REVISED)
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)
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/
>> 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
>> 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 =
>> That would match what you did on lines 99-100.
> Yes, I changed that.
>> 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.
>> No comments.
>>> 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/
More information about the hotspot-runtime-dev