• Type: Bug
• Component: hotspot
• Sub-Component: runtime
• Affected Version: 12

• Priority: P3
• Status: Resolved
• Resolution: Fixed

• Submitted: 2018-08-10
• Updated: 2019-05-28
• Resolved: 2018-12-10

JDK 12
12 b24Fixed

Fixed bug title. Otherwise it's very hard to filter for emails auto-generated by JBS. 14-08-2018 Kim Barrett's comment: src/hotspot/share/classfile/symbolTable.cpp +void SymbolTable::delete_symbol(Symbol* sym) { + if (sym->refcount() == PERM_REFCOUNT) { + MutexLocker ml(SymbolTable_lock); // Protect arena + // Deleting permanent symbol should not occur very often (insert race condition), + // so log it. + _log_trace_symboltable_helper(sym, "Freeing permanent symbol"); + if (!arena()->Afree(sym, sym->size())) { + _log_trace_symboltable_helper(sym, "Leaked permanent symbol"); + } + } else { + delete sym; + } +} I'm surprised there isn't an assert that sym->refcount() == 0 in the else clause. That would also obviate the need for the assert in the only caller (SymbolTableConfig::free_node). my comment: I like the assert in SymbolTableConfig::free_node better, which also handles "sym->refcount() == PERM_REFCOUNT", so when we get to "SymbolTable::delete_symbol��� we know we���re good. Kim Barrett's comment: Sorry, but I'm not convinced. I think it's better to have the assert near the place that actually cares, which is delete_symbol. I think the assert check for PERM_REFCOUNT is just redundant. I also think the associated comment would be better placed in delete_symbol. my comment: #1 static void free_node(void* memory, Symbol* const& value) { does more than actually deletes the symbol. It also calls ���SymbolTableHash::BaseConfig::free_node���, so it needs the ���assert(ym->refcount() == 0��� itself. It���s the only place that calls "SymbolTable::delete_symbol���, so it can guarantee that ���delete_symbol��� is called with a dead symbol. #2 ���SymbolTable::delete_symbol��� is a wrapper for ���delete symbol���, which is Symbol private. But if I could, I would call it directly in ���free_node��� and then the assert would have to be in ���free_node��� 10-08-2018 Kim Barrett's comment: Using macros for constants is commonly considered poor style. The HotSpot style guide says "You can almost always use an inline function or class instead of a macro. Use a macro only when you really need it." While this doesn't explicitly mention object macros, the second sentence certainly applies. Using enums is not a good alternative: JDK-8153116. That said, I won't object to making this a cleanup item for later, esp. since it also applies to StringTable. 10-08-2018 Kim Barrett's comment: _alt_hash is not subject to concurrent modification, which is a good thing because there is lots of code that is not set up to cope with that possibility. It is only set by rehash_table, which is only called from safepoint cleanup. Hm, but looking at that makes me nervous. We're running various cleanup tasks in parallel. What happens if one of the other parallel tasks needs to look up a symbol? That seems like it would lead to bad things if it can happen. And it's not entirely obvious to me that it can't happen. Similarly for stringtable. Given the way it's used, it seems like it should be a StringTable ordinary member rather than a static member, and the new table produced by rehashing (that wants it turned on) should be constructed with it being true. 10-08-2018 Kim Barrett's comment: ------------------------------------------------------------------------------ src/hotspot/share/classfile/symbolTable.cpp 727 Atomic::add((size_t)stdc._processed, &_symbols_counted); Why not make SymbolTableDeleteCheck::_processed by size_t, making this cast unnecessary? And why not make SymbolTableDoDelete::_deleted also size_t. These changes would require making the logging call at the end of clean_dead_entries use SIZE_FORMAT rather than INT32_FORMAT. 10-08-2018 Kim Barrett's comment: ------------------------------------------------------------------------------ src/hotspot/share/classfile/stringTable.hpp 87 size_t add_items_count_to_clean(size_t ndead); This name change doesn't seem right. 10-08-2018