8180048: Interned string and symbol table leak memory during parallel unlinking

Mon, 15 May 2017 12:20:15 +0200

author
tschatzl
date
Mon, 15 May 2017 12:20:15 +0200
changeset 8766
ce9a710b0f63
parent 8764
0bd600d6d77b
child 8767
7b8c8cd1ee71

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

src/share/vm/classfile/symbolTable.cpp file | annotate | diff | comparison | revisions
src/share/vm/classfile/symbolTable.hpp file | annotate | diff | comparison | revisions
src/share/vm/runtime/vmStructs.cpp file | annotate | diff | comparison | revisions
src/share/vm/utilities/hashtable.cpp file | annotate | diff | comparison | revisions
src/share/vm/utilities/hashtable.hpp file | annotate | diff | comparison | revisions
     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);

mercurial