Request for review: 7150058: Allocate symbols from null boot loader to an arena for NMT

[Tom Rodriguez]

On Mar 8, 2012, at 12:32 PM, Coleen Phillimore wrote:

> 
> 
> On 3/8/2012 12:51 PM, Tom Rodriguez wrote:
>> On Mar 8, 2012, at 7:55 AM, Coleen Phillimore wrote:
>> 
>>> Please review this change to allocate symbols from the boot class loader into a  global arena.   This change reduces reporting traffic at startup for the native memory tracking project in development.  It is also useful for class data sharing in the permgen elimination project, also in development.   I had to increase the SharedMiscSize because ResourceObj in debug mode is 2 extra pointers.
>> I think the decision about where a symbol is allocated should be inside of operator new instead of up in SymbolTable.cpp.  Why not pass the class loader all the way through?
> 
> I thought this is a lot cleaner but the lookup() calls call basic_add() and they don't have a class loader passed down.   We don't really want a null class loader in this case because many lookup calls are for temporary symbols, or for symbols constructed to find a class.   These temporary symbols would be good to remove.

That makes sense but that intent isn't clear from the code as written.  Some comments and maybe renaming would make that intent more clear.  There are distinct paths for klass symbols and for one off symbols but they are named very similarly, which was fine when they did similar things.

I think the Arena might make more sense in SymbolTable given how intimately if knows where things are being allocated.

>> Why is there a path where c_heap is being forced?
>> 
>> +  // Create a new symbol.
>> +  Symbol* sym = allocate_symbol(name, len, true, CHECK_NULL);
>> +  assert(sym->equals((char*)name, len), "symbol must be properly initialized");
> 
> This is the basic_add() case that comes from lookup().  There is one path below where we know the class loader.   I started to hook it up, then decided that it wasn't worth changing all the basic_add() calls to pass "true" in the cases where we don't know what class loader the symbol is from.

Aren't there a bunch of symbols that are looked up as part of vmSymbols and such that are also very long lived?

>> Maybe this isn't wired up?
>> 
>> +      bool c_heap = class_loader() != NULL;
>>        Symbol* sym = table->basic_add(index, (u1*)names[i], lengths[i],
>> -                                       hashValues[i], CHECK);
>> +                                     hashValues[i], CHECK);
> 
> I didn't end up wiring this up, see above.  I deleted this line.  This case is when we have a list of symbols and for some reason the batch basic add fails.  The only case that would happen is if we couldn't get a new arena, so it sort of makes sense to individually allocate the symbols in C heap.

Ok.

tom

> 
> Thanks,
> Coleen
>> tom
>> 
>>> open webrev at http://cr.openjdk.java.net/~coleenp/symbol-arena
>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7150058
>>> 
>>> Thanks,
>>> coleen
>>>