OpenJDK / jdk / hs
changeset 45114:45644c5f6b8e
8180048: Interned string and symbol table leak memory during parallel unlinking
Summary: Make appending found dead BasicHashtableEntrys to the free list atomic.
Reviewed-by: ehelin, shade, coleenp
author | tschatzl |
---|---|
date | Mon, 15 May 2017 12:20:15 +0200 |
parents | 1527aaaeb42d |
children | e3a622b2b7db |
files | hotspot/src/share/vm/classfile/stringTable.cpp hotspot/src/share/vm/classfile/stringTable.hpp hotspot/src/share/vm/classfile/symbolTable.cpp hotspot/src/share/vm/classfile/symbolTable.hpp hotspot/src/share/vm/runtime/vmStructs.cpp hotspot/src/share/vm/utilities/hashtable.cpp hotspot/src/share/vm/utilities/hashtable.hpp |
diffstat | 7 files changed, 97 insertions(+), 29 deletions(-) [+] |
line wrap: on
line diff
--- a/hotspot/src/share/vm/classfile/stringTable.cpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/classfile/stringTable.cpp Mon May 15 12:20:15 2017 +0200 @@ -314,7 +314,11 @@ } void StringTable::unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int* processed, int* removed) { - buckets_unlink_or_oops_do(is_alive, f, 0, the_table()->table_size(), processed, removed); + BucketUnlinkContext context; + buckets_unlink_or_oops_do(is_alive, f, 0, the_table()->table_size(), &context); + _the_table->bulk_free_entries(&context); + *processed = context._num_processed; + *removed = context._num_removed; } void StringTable::possibly_parallel_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int* processed, int* removed) { @@ -323,6 +327,7 @@ assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint"); const int limit = the_table()->table_size(); + BucketUnlinkContext context; for (;;) { // Grab next set of buckets to scan int start_idx = Atomic::add(ClaimChunkSize, &_parallel_claimed_idx) - ClaimChunkSize; @@ -332,8 +337,11 @@ } int end_idx = MIN2(limit, start_idx + ClaimChunkSize); - buckets_unlink_or_oops_do(is_alive, f, start_idx, end_idx, processed, removed); + buckets_unlink_or_oops_do(is_alive, f, start_idx, end_idx, &context); } + _the_table->bulk_free_entries(&context); + *processed = context._num_processed; + *removed = context._num_removed; } void StringTable::buckets_oops_do(OopClosure* f, int start_idx, int end_idx) { @@ -359,7 +367,7 @@ } } -void StringTable::buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, int* processed, int* removed) { +void StringTable::buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, BucketUnlinkContext* context) { const int limit = the_table()->table_size(); assert(0 <= start_idx && start_idx <= limit, @@ -383,10 +391,9 @@ p = entry->next_addr(); } else { *p = entry->next(); - the_table()->free_entry(entry); - (*removed)++; + context->free_entry(entry); } - (*processed)++; + context->_num_processed++; entry = *p; } }
--- a/hotspot/src/share/vm/classfile/stringTable.hpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/classfile/stringTable.hpp Mon May 15 12:20:15 2017 +0200 @@ -61,9 +61,13 @@ // Apply the give oop closure to the entries to the buckets // in the range [start_idx, end_idx). static void buckets_oops_do(OopClosure* f, int start_idx, int end_idx); + + typedef StringTable::BucketUnlinkContext BucketUnlinkContext; // Unlink or apply the give oop closure to the entries to the buckets - // in the range [start_idx, end_idx). - static void buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, int* processed, int* removed); + // in the range [start_idx, end_idx). Unlinked bucket entries are collected in the given + // context to be freed later. + // This allows multiple threads to work on the table at once. + static void buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, BucketUnlinkContext* context); // Hashing algorithm, used as the hash value used by the // StringTable for bucket selection and comparison (stored in the
--- a/hotspot/src/share/vm/classfile/symbolTable.cpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/classfile/symbolTable.cpp Mon May 15 12:20:15 2017 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -98,7 +98,7 @@ int SymbolTable::_symbols_counted = 0; volatile int SymbolTable::_parallel_claimed_idx = 0; -void SymbolTable::buckets_unlink(int start_idx, int end_idx, int* processed, int* removed) { +void SymbolTable::buckets_unlink(int start_idx, int end_idx, BucketUnlinkContext* context) { for (int i = start_idx; i < end_idx; ++i) { HashtableEntry<Symbol*, mtSymbol>** p = the_table()->bucket_addr(i); HashtableEntry<Symbol*, mtSymbol>* entry = the_table()->bucket(i); @@ -111,15 +111,14 @@ break; } Symbol* s = entry->literal(); - (*processed)++; + context->_num_processed++; assert(s != NULL, "just checking"); // If reference count is zero, remove. if (s->refcount() == 0) { assert(!entry->is_shared(), "shared entries should be kept live"); delete s; - (*removed)++; *p = entry->next(); - the_table()->free_entry(entry); + context->free_entry(entry); } else { p = entry->next_addr(); } @@ -132,17 +131,20 @@ // Remove unreferenced symbols from the symbol table // This is done late during GC. void SymbolTable::unlink(int* processed, int* removed) { - size_t memory_total = 0; - buckets_unlink(0, the_table()->table_size(), processed, removed); - _symbols_removed += *removed; - _symbols_counted += *processed; + BucketUnlinkContext context; + buckets_unlink(0, the_table()->table_size(), &context); + _the_table->bulk_free_entries(&context); + *processed = context._num_processed; + *removed = context._num_removed; + + _symbols_removed = context._num_removed; + _symbols_counted = context._num_processed; } void SymbolTable::possibly_parallel_unlink(int* processed, int* removed) { const int limit = the_table()->table_size(); - size_t memory_total = 0; - + BucketUnlinkContext context; for (;;) { // Grab next set of buckets to scan int start_idx = Atomic::add(ClaimChunkSize, &_parallel_claimed_idx) - ClaimChunkSize; @@ -152,10 +154,15 @@ } int end_idx = MIN2(limit, start_idx + ClaimChunkSize); - buckets_unlink(start_idx, end_idx, processed, removed); + buckets_unlink(start_idx, end_idx, &context); } - Atomic::add(*processed, &_symbols_counted); - Atomic::add(*removed, &_symbols_removed); + + _the_table->bulk_free_entries(&context); + *processed = context._num_processed; + *removed = context._num_removed; + + Atomic::add(context._num_processed, &_symbols_counted); + Atomic::add(context._num_removed, &_symbols_removed); } // Create a new table and using alternate hash code, populate the new table
--- a/hotspot/src/share/vm/classfile/symbolTable.hpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/classfile/symbolTable.hpp Mon May 15 12:20:15 2017 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -154,8 +154,11 @@ static volatile int _parallel_claimed_idx; - // Release any dead symbols - static void buckets_unlink(int start_idx, int end_idx, int* processed, int* removed); + typedef SymbolTable::BucketUnlinkContext BucketUnlinkContext; + // Release any dead symbols. Unlinked bucket entries are collected in the given + // context to be freed later. + // This allows multiple threads to work on the table at once. + static void buckets_unlink(int start_idx, int end_idx, BucketUnlinkContext* context); public: enum { symbol_alloc_batch_size = 8,
--- a/hotspot/src/share/vm/runtime/vmStructs.cpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/runtime/vmStructs.cpp Mon May 15 12:20:15 2017 +0200 @@ -673,7 +673,7 @@ \ nonstatic_field(BasicHashtable<mtInternal>, _table_size, int) \ nonstatic_field(BasicHashtable<mtInternal>, _buckets, HashtableBucket<mtInternal>*) \ - nonstatic_field(BasicHashtable<mtInternal>, _free_list, BasicHashtableEntry<mtInternal>*) \ + volatile_nonstatic_field(BasicHashtable<mtInternal>, _free_list, BasicHashtableEntry<mtInternal>*) \ nonstatic_field(BasicHashtable<mtInternal>, _first_free_entry, char*) \ nonstatic_field(BasicHashtable<mtInternal>, _end_block, char*) \ nonstatic_field(BasicHashtable<mtInternal>, _entry_size, int) \
--- a/hotspot/src/share/vm/utilities/hashtable.cpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/utilities/hashtable.cpp Mon May 15 12:20:15 2017 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -173,6 +173,35 @@ } } +template <MEMFLAGS F> void BasicHashtable<F>::BucketUnlinkContext::free_entry(BasicHashtableEntry<F>* entry) { + entry->set_next(_removed_head); + _removed_head = entry; + if (_removed_tail == NULL) { + _removed_tail = entry; + } + _num_removed++; +} + +template <MEMFLAGS F> void BasicHashtable<F>::bulk_free_entries(BucketUnlinkContext* context) { + if (context->_num_removed == 0) { + assert(context->_removed_head == NULL && context->_removed_tail == NULL, + "Zero entries in the unlink context, but elements linked from " PTR_FORMAT " to " PTR_FORMAT, + p2i(context->_removed_head), p2i(context->_removed_tail)); + return; + } + + // MT-safe add of the list of BasicHashTableEntrys from the context to the free list. + BasicHashtableEntry<F>* current = _free_list; + while (true) { + context->_removed_tail->set_next(current); + BasicHashtableEntry<F>* old = (BasicHashtableEntry<F>*)Atomic::cmpxchg_ptr(context->_removed_head, &_free_list, current); + if (old == current) { + break; + } + current = old; + } + Atomic::add(-context->_num_removed, &_number_of_entries); +} // Copy the table to the shared space.
--- a/hotspot/src/share/vm/utilities/hashtable.hpp Thu May 11 14:13:59 2017 -0700 +++ b/hotspot/src/share/vm/utilities/hashtable.hpp Mon May 15 12:20:15 2017 +0200 @@ -173,11 +173,11 @@ // Instance variables int _table_size; HashtableBucket<F>* _buckets; - BasicHashtableEntry<F>* _free_list; + BasicHashtableEntry<F>* volatile _free_list; char* _first_free_entry; char* _end_block; int _entry_size; - int _number_of_entries; + volatile int _number_of_entries; protected: @@ -225,6 +225,24 @@ // Free the buckets in this hashtable void free_buckets(); + // Helper data structure containing context for the bucket entry unlink process, + // storing the unlinked buckets in a linked list. + // Also avoids the need to pass around these four members as parameters everywhere. + struct BucketUnlinkContext { + int _num_processed; + int _num_removed; + // Head and tail pointers for the linked list of removed entries. + BasicHashtableEntry<F>* _removed_head; + BasicHashtableEntry<F>* _removed_tail; + + BucketUnlinkContext() : _num_processed(0), _num_removed(0), _removed_head(NULL), _removed_tail(NULL) { + } + + void free_entry(BasicHashtableEntry<F>* entry); + }; + // Add of bucket entries linked together in the given context to the global free list. This method + // is mt-safe wrt. to other calls of this method. + void bulk_free_entries(BucketUnlinkContext* context); public: int table_size() { return _table_size; } void set_entry(int index, BasicHashtableEntry<F>* entry);