Mon, 15 May 2017 12:20:15 +0200
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
1.1 --- a/src/share/vm/classfile/symbolTable.cpp Fri May 05 06:07:11 2017 -0700 1.2 +++ b/src/share/vm/classfile/symbolTable.cpp Mon May 15 12:20:15 2017 +0200 1.3 @@ -1,5 +1,5 @@ 1.4 /* 1.5 - * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved. 1.6 + * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved. 1.7 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. 1.8 * 1.9 * This code is free software; you can redistribute it and/or modify it 1.10 @@ -96,7 +96,7 @@ 1.11 int SymbolTable::_symbols_counted = 0; 1.12 volatile int SymbolTable::_parallel_claimed_idx = 0; 1.13 1.14 -void SymbolTable::buckets_unlink(int start_idx, int end_idx, int* processed, int* removed, size_t* memory_total) { 1.15 +void SymbolTable::buckets_unlink(int start_idx, int end_idx, BucketUnlinkContext* context, size_t* memory_total) { 1.16 for (int i = start_idx; i < end_idx; ++i) { 1.17 HashtableEntry<Symbol*, mtSymbol>** p = the_table()->bucket_addr(i); 1.18 HashtableEntry<Symbol*, mtSymbol>* entry = the_table()->bucket(i); 1.19 @@ -110,15 +110,14 @@ 1.20 } 1.21 Symbol* s = entry->literal(); 1.22 (*memory_total) += s->size(); 1.23 - (*processed)++; 1.24 + context->_num_processed++; 1.25 assert(s != NULL, "just checking"); 1.26 // If reference count is zero, remove. 1.27 if (s->refcount() == 0) { 1.28 assert(!entry->is_shared(), "shared entries should be kept live"); 1.29 delete s; 1.30 - (*removed)++; 1.31 *p = entry->next(); 1.32 - the_table()->free_entry(entry); 1.33 + context->free_entry(entry); 1.34 } else { 1.35 p = entry->next_addr(); 1.36 } 1.37 @@ -132,9 +131,14 @@ 1.38 // This is done late during GC. 1.39 void SymbolTable::unlink(int* processed, int* removed) { 1.40 size_t memory_total = 0; 1.41 - buckets_unlink(0, the_table()->table_size(), processed, removed, &memory_total); 1.42 - _symbols_removed += *removed; 1.43 - _symbols_counted += *processed; 1.44 + BucketUnlinkContext context; 1.45 + buckets_unlink(0, the_table()->table_size(), &context, &memory_total); 1.46 + _the_table->bulk_free_entries(&context); 1.47 + *processed = context._num_processed; 1.48 + *removed = context._num_removed; 1.49 + 1.50 + _symbols_removed = context._num_removed; 1.51 + _symbols_counted = context._num_processed; 1.52 // Exclude printing for normal PrintGCDetails because people parse 1.53 // this output. 1.54 if (PrintGCDetails && Verbose && WizardMode) { 1.55 @@ -148,6 +152,7 @@ 1.56 1.57 size_t memory_total = 0; 1.58 1.59 + BucketUnlinkContext context; 1.60 for (;;) { 1.61 // Grab next set of buckets to scan 1.62 int start_idx = Atomic::add(ClaimChunkSize, &_parallel_claimed_idx) - ClaimChunkSize; 1.63 @@ -157,10 +162,15 @@ 1.64 } 1.65 1.66 int end_idx = MIN2(limit, start_idx + ClaimChunkSize); 1.67 - buckets_unlink(start_idx, end_idx, processed, removed, &memory_total); 1.68 + buckets_unlink(start_idx, end_idx, &context, &memory_total); 1.69 } 1.70 - Atomic::add(*processed, &_symbols_counted); 1.71 - Atomic::add(*removed, &_symbols_removed); 1.72 + 1.73 + _the_table->bulk_free_entries(&context); 1.74 + *processed = context._num_processed; 1.75 + *removed = context._num_removed; 1.76 + 1.77 + Atomic::add(context._num_processed, &_symbols_counted); 1.78 + Atomic::add(context._num_removed, &_symbols_removed); 1.79 // Exclude printing for normal PrintGCDetails because people parse 1.80 // this output. 1.81 if (PrintGCDetails && Verbose && WizardMode) { 1.82 @@ -811,7 +821,11 @@ 1.83 } 1.84 1.85 void StringTable::unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int* processed, int* removed) { 1.86 - buckets_unlink_or_oops_do(is_alive, f, 0, the_table()->table_size(), processed, removed); 1.87 + BucketUnlinkContext context; 1.88 + buckets_unlink_or_oops_do(is_alive, f, 0, the_table()->table_size(), &context); 1.89 + _the_table->bulk_free_entries(&context); 1.90 + *processed = context._num_processed; 1.91 + *removed = context._num_removed; 1.92 } 1.93 1.94 void StringTable::possibly_parallel_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int* processed, int* removed) { 1.95 @@ -820,6 +834,7 @@ 1.96 assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint"); 1.97 const int limit = the_table()->table_size(); 1.98 1.99 + BucketUnlinkContext context; 1.100 for (;;) { 1.101 // Grab next set of buckets to scan 1.102 int start_idx = Atomic::add(ClaimChunkSize, &_parallel_claimed_idx) - ClaimChunkSize; 1.103 @@ -829,8 +844,11 @@ 1.104 } 1.105 1.106 int end_idx = MIN2(limit, start_idx + ClaimChunkSize); 1.107 - buckets_unlink_or_oops_do(is_alive, f, start_idx, end_idx, processed, removed); 1.108 + buckets_unlink_or_oops_do(is_alive, f, start_idx, end_idx, &context); 1.109 } 1.110 + _the_table->bulk_free_entries(&context); 1.111 + *processed = context._num_processed; 1.112 + *removed = context._num_removed; 1.113 } 1.114 1.115 void StringTable::buckets_oops_do(OopClosure* f, int start_idx, int end_idx) { 1.116 @@ -856,7 +874,7 @@ 1.117 } 1.118 } 1.119 1.120 -void StringTable::buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, int* processed, int* removed) { 1.121 +void StringTable::buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, BucketUnlinkContext* context) { 1.122 const int limit = the_table()->table_size(); 1.123 1.124 assert(0 <= start_idx && start_idx <= limit, 1.125 @@ -880,10 +898,9 @@ 1.126 p = entry->next_addr(); 1.127 } else { 1.128 *p = entry->next(); 1.129 - the_table()->free_entry(entry); 1.130 - (*removed)++; 1.131 + context->free_entry(entry); 1.132 } 1.133 - (*processed)++; 1.134 + context->_num_processed++; 1.135 entry = *p; 1.136 } 1.137 }
2.1 --- a/src/share/vm/classfile/symbolTable.hpp Fri May 05 06:07:11 2017 -0700 2.2 +++ b/src/share/vm/classfile/symbolTable.hpp Mon May 15 12:20:15 2017 +0200 2.3 @@ -1,5 +1,5 @@ 2.4 /* 2.5 - * Copyright (c) 1997, 2013, Oracle and/or its affiliates. All rights reserved. 2.6 + * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved. 2.7 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. 2.8 * 2.9 * This code is free software; you can redistribute it and/or modify it 2.10 @@ -124,8 +124,11 @@ 2.11 2.12 static volatile int _parallel_claimed_idx; 2.13 2.14 - // Release any dead symbols 2.15 - static void buckets_unlink(int start_idx, int end_idx, int* processed, int* removed, size_t* memory_total); 2.16 + typedef SymbolTable::BucketUnlinkContext BucketUnlinkContext; 2.17 + // Release any dead symbols. Unlinked bucket entries are collected in the given 2.18 + // context to be freed later. 2.19 + // This allows multiple threads to work on the table at once. 2.20 + static void buckets_unlink(int start_idx, int end_idx, BucketUnlinkContext* context, size_t* memory_total); 2.21 public: 2.22 enum { 2.23 symbol_alloc_batch_size = 8, 2.24 @@ -274,9 +277,13 @@ 2.25 // Apply the give oop closure to the entries to the buckets 2.26 // in the range [start_idx, end_idx). 2.27 static void buckets_oops_do(OopClosure* f, int start_idx, int end_idx); 2.28 + 2.29 + typedef StringTable::BucketUnlinkContext BucketUnlinkContext; 2.30 // Unlink or apply the give oop closure to the entries to the buckets 2.31 - // in the range [start_idx, end_idx). 2.32 - static void buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, int* processed, int* removed); 2.33 + // in the range [start_idx, end_idx). Unlinked bucket entries are collected in the given 2.34 + // context to be freed later. 2.35 + // This allows multiple threads to work on the table at once. 2.36 + static void buckets_unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f, int start_idx, int end_idx, BucketUnlinkContext* context); 2.37 2.38 StringTable() : RehashableHashtable<oop, mtSymbol>((int)StringTableSize, 2.39 sizeof (HashtableEntry<oop, mtSymbol>)) {}
3.1 --- a/src/share/vm/runtime/vmStructs.cpp Fri May 05 06:07:11 2017 -0700 3.2 +++ b/src/share/vm/runtime/vmStructs.cpp Mon May 15 12:20:15 2017 +0200 3.3 @@ -704,7 +704,7 @@ 3.4 \ 3.5 nonstatic_field(BasicHashtable<mtInternal>, _table_size, int) \ 3.6 nonstatic_field(BasicHashtable<mtInternal>, _buckets, HashtableBucket<mtInternal>*) \ 3.7 - nonstatic_field(BasicHashtable<mtInternal>, _free_list, BasicHashtableEntry<mtInternal>*) \ 3.8 + volatile_nonstatic_field(BasicHashtable<mtInternal>, _free_list, BasicHashtableEntry<mtInternal>*) \ 3.9 nonstatic_field(BasicHashtable<mtInternal>, _first_free_entry, char*) \ 3.10 nonstatic_field(BasicHashtable<mtInternal>, _end_block, char*) \ 3.11 nonstatic_field(BasicHashtable<mtInternal>, _entry_size, int) \
4.1 --- a/src/share/vm/utilities/hashtable.cpp Fri May 05 06:07:11 2017 -0700 4.2 +++ b/src/share/vm/utilities/hashtable.cpp Mon May 15 12:20:15 2017 +0200 4.3 @@ -1,5 +1,5 @@ 4.4 /* 4.5 - * Copyright (c) 2003, 2014, Oracle and/or its affiliates. All rights reserved. 4.6 + * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights reserved. 4.7 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. 4.8 * 4.9 * This code is free software; you can redistribute it and/or modify it 4.10 @@ -172,6 +172,35 @@ 4.11 } 4.12 } 4.13 4.14 +template <MEMFLAGS F> void BasicHashtable<F>::BucketUnlinkContext::free_entry(BasicHashtableEntry<F>* entry) { 4.15 + entry->set_next(_removed_head); 4.16 + _removed_head = entry; 4.17 + if (_removed_tail == NULL) { 4.18 + _removed_tail = entry; 4.19 + } 4.20 + _num_removed++; 4.21 +} 4.22 + 4.23 +template <MEMFLAGS F> void BasicHashtable<F>::bulk_free_entries(BucketUnlinkContext* context) { 4.24 + if (context->_num_removed == 0) { 4.25 + assert(context->_removed_head == NULL && context->_removed_tail == NULL, 4.26 + err_msg("Zero entries in the unlink context, but elements linked from " PTR_FORMAT " to " PTR_FORMAT, 4.27 + p2i(context->_removed_head), p2i(context->_removed_tail))); 4.28 + return; 4.29 + } 4.30 + 4.31 + // MT-safe add of the list of BasicHashTableEntrys from the context to the free list. 4.32 + BasicHashtableEntry<F>* current = _free_list; 4.33 + while (true) { 4.34 + context->_removed_tail->set_next(current); 4.35 + BasicHashtableEntry<F>* old = (BasicHashtableEntry<F>*)Atomic::cmpxchg_ptr(context->_removed_head, &_free_list, current); 4.36 + if (old == current) { 4.37 + break; 4.38 + } 4.39 + current = old; 4.40 + } 4.41 + Atomic::add(-context->_num_removed, &_number_of_entries); 4.42 +} 4.43 4.44 // Copy the table to the shared space. 4.45
5.1 --- a/src/share/vm/utilities/hashtable.hpp Fri May 05 06:07:11 2017 -0700 5.2 +++ b/src/share/vm/utilities/hashtable.hpp Mon May 15 12:20:15 2017 +0200 5.3 @@ -164,11 +164,11 @@ 5.4 // Instance variables 5.5 int _table_size; 5.6 HashtableBucket<F>* _buckets; 5.7 - BasicHashtableEntry<F>* _free_list; 5.8 + BasicHashtableEntry<F>* volatile _free_list; 5.9 char* _first_free_entry; 5.10 char* _end_block; 5.11 int _entry_size; 5.12 - int _number_of_entries; 5.13 + volatile int _number_of_entries; 5.14 5.15 protected: 5.16 5.17 @@ -215,6 +215,24 @@ 5.18 // Free the buckets in this hashtable 5.19 void free_buckets(); 5.20 5.21 + // Helper data structure containing context for the bucket entry unlink process, 5.22 + // storing the unlinked buckets in a linked list. 5.23 + // Also avoids the need to pass around these four members as parameters everywhere. 5.24 + struct BucketUnlinkContext { 5.25 + int _num_processed; 5.26 + int _num_removed; 5.27 + // Head and tail pointers for the linked list of removed entries. 5.28 + BasicHashtableEntry<F>* _removed_head; 5.29 + BasicHashtableEntry<F>* _removed_tail; 5.30 + 5.31 + BucketUnlinkContext() : _num_processed(0), _num_removed(0), _removed_head(NULL), _removed_tail(NULL) { 5.32 + } 5.33 + 5.34 + void free_entry(BasicHashtableEntry<F>* entry); 5.35 + }; 5.36 + // Add of bucket entries linked together in the given context to the global free list. This method 5.37 + // is mt-safe wrt. to other calls of this method. 5.38 + void bulk_free_entries(BucketUnlinkContext* context); 5.39 public: 5.40 int table_size() { return _table_size; } 5.41 void set_entry(int index, BasicHashtableEntry<F>* entry);