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

[Coleen Phillimore]

Tom,  Thank you for the code review so far.   Your suggestion to move 
the arena into symbolTable was good, so I made that change.  I also 
added new_permanent_symbol() for the cases where it is known that the 
symbol is going to be long-lived.   See answers inlined.   The new 
webrev is at:

http://oklahoma.us.oracle.com/~cphillim/webrev/symbol-arena2/

Also, I need another reviewer!

On 3/8/2012 7:35 PM, Tom Rodriguez wrote:
> 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.
>
Agree.
>>> 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?
>

Yes there are.  I added new_permanent_symbol for the places where the 
symbol allocation is known to be long-lived, which starts the refcount 
out at -1.

>>> 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.
I ended up rewiring t his again because I had to add the c_heap object 
to basic_add() for the new_permanent_symbol case.

Thanks,
Coleen
> 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
>>>>