Review request for 8022524 : Memory leaks in nashorn sources and tests found by jhat analysis

[A. Sundararajan]

Updated based on comments from Attila.

Update webrev @ http://cr.openjdk.java.net/~sundar/8022524/webrev.02/

-Sundar

On Thursday 08 August 2013 10:54 AM, A. Sundararajan wrote:
> Thanks. I got it. Basically InvokeByName/MethodHandle may be created 
> by Callable atmost twice -- and the second one will be thrown away.
>
> I'll make these changes.
>
> -Sundar
>
> On Thursday 08 August 2013 10:41 AM, Attila Szegedi wrote:
>> On Aug 8, 2013, at 5:45 AM, A. Sundararajan 
>> <sundararajan.athijegannathan at oracle.com> wrote:
>>
>>> I'd like to lazily initialize InvokeByName and dynamic method 
>>> handles in global instance. I am not sure of the refactoring in 
>>> NativeArray that you suggested.
>>>
>>> Are you talking about these?
>>>
>>>             private final MethodHandle someInvoker = 
>>> getSOME_CALLBACK_INVOKER();
>>>
>>> If I pass key to a refactored method, I've to do if..else on object 
>>> identity check.. Or am I missing something?
>> This is what I had in mind:
>>
>> private static MethodHandle getEVERY_CALLBACK_INVOKER() {
>>      return createIteratorCallbackInvoker(EVERY_CALLBACK_INVOKER, 
>> boolean.class);
>> }
>>
>> private static MethodHandle getSOME_CALLBACK_INVOKER() {
>>      return createIteratorCallbackInvoker(SOME_CALLBACK_INVOKER, 
>> boolean.class);
>> }
>>
>> ...
>>
>> private static createIteratorCallbackInvoker(final Object key, final 
>> Class<?> rtype) {
>>      return Global.instance().getDynamicInvoker(key,
>>              new Callable<MethodHandle>() {
>>                  @Override
>>                  public MethodHandle call() {
>>                      return createIteratorCallbackInvoker(rtype);
>>                  }
>>              });
>> }
>>
>>> Also, on avoiding synchronized in Global.java. If we'd like to avoid 
>>> jdk8 specific API/syntax in nashorn, I can't use computeIfAbsent. 
>>> But putIfAbsent forces computing the value... Again, am I missing 
>>> something?
>> private final ConcurrentMap<Object, InvokeByName> namedInvokers = new 
>> ConcurrentHashMap<>();
>>
>> @Override
>> public InvokeByName getInvokeByName(final Object key, final 
>> Callable<InvokeByName> creator) {
>>      final InvokeByName invoke = namedInvokers.get(key);
>>      if(invoke != null) {
>>          return invoke;
>>      }
>>      final InvokeByName newInvoke = creator.call();
>>      final InvokeByName existingInvoke = 
>> namedInvokers.putIfAbsent(key, newInvoke);
>>      return existingInvoke != null ? existingInvoke : newInvoke;
>> }
>>
>>> thanks,
>>> -Sundar
>>>
>>> On Thursday 08 August 2013 08:56 AM, A. Sundararajan wrote:
>>>> On Thursday 08 August 2013 02:29 AM, Attila Szegedi wrote:
>>>>> - CompileUnit: While making fields non-final and nulling out 
>>>>> fields is certainly a solution, I don't like it as it feels 
>>>>> fragile - you end up with an object that has a member nulled out, 
>>>>> and what if something later would want to depend on it etc. As an 
>>>>> example, consider CompileUnit, which now has its ClassEmitter 
>>>>> nulled out. Seems like architecturally it's a better idea is to 
>>>>> remove the field from the CompileUnit altogether, and use a 
>>>>> composite object being a tuple of (CompileUnit, ClassEmitter) in 
>>>>> the compiler, and only pass down the CompileUnit part of the tuple 
>>>>> to things in the IR package that require it.
>>>> While code can be refactored for a longer term, as of now, it does 
>>>> leak memory. Moment class is loaded, we don't need lots of info 
>>>> maintained by ASM's ClassEmitter. I suggest we go with short term 
>>>> solution and revisit refactoring changes to 
>>>> FunctionNode/CompileUnit/Compiler later.
>>>>
>>>>> - Another issue I have is with synchronization in the Global 
>>>>> object; I'd rather use a ConcurrentMap and the (new for Java 8) 
>>>>> computeIfAbsent() method. 
>>>>> <http://download.java.net/jdk8/docs/api/java/util/Map.html#computeIfAbsent(K, 
>>>>> java.util.function.Function)>. If you don't want to rely on 
>>>>> computeIfAbsent() (but I don't see why wouldn't you, frankly), you 
>>>>> could still use a composition of get() and putIfAbsent().
>>>> We still don't use any jdk8 specific API in nashorn codebase yet (I 
>>>> believe). I'll restructure this with older API.
>>>>> - In NativeArray, you could factor out the pattern of getting an 
>>>>> invoker for an iterator callback repeated across 4 methods into a 
>>>>> method taking a key and a return type.
>>>> Will do.
>>>>
>>>>> - Ostensibly, NativeObject could just use Global.TO_STRING instead 
>>>>> of having its own now. Not too convinced about this, as these 
>>>>> things sort-of represent call sites, so maybe it's okay as it is.
>>>> Yes - it is a different callsite (although I doubt how much 
>>>> InvokeByName and dynamic invokers help now!)
>>>>> - We still keep GlobalObject interface around?
>>>> Yes - we do. That calls for more refactorings. As I said, I'd like 
>>>> to keep it minimal (as much as possible) for now.
>>>>> - Why does RecompilableScriptFunctionData.ensureHasAllocator have 
>>>>> to be synchronized? If we absolutely need atomic updates to the 
>>>>> allocator field, I'd consider using an AtomicReference for it 
>>>>> instead. Having synchronization in path of every "new SomeClass()" 
>>>>> bothers me. Even if it's completely unsynced and the field is not 
>>>>> volatile, we only "risk" creating the method handle multiple 
>>>>> times; shouldn't be a big deal as we're (a) rarely multithreaded 
>>>>> and (b) it's idempotent. So, I'd rather choose a bit of a 
>>>>> statistical redundancy than a certain performance hit.
>>>>>
>>>>> - Why does ensureCodeGenerated have to be synchronized? Can the 
>>>>> modifications of fields possibly occur on multiple threads? I 
>>>>> mean, functionNode.canSpecialize() will be determined at first 
>>>>> execution and fields nulled out. Also, wouldn't a second call to 
>>>>> ensureCodeGenerated() after functionNode was nulled out (if that's 
>>>>> possible) result in a NPE on functionNode.isLazy(), or is this 
>>>>> guarded by !code.isEmpty()? At least this synchronization only 
>>>>> happens once on every linking event and not on every invocation, 
>>>>> unlike allocate() but I still don't really see the necessity.
>>>> I'll check again.
>>>>
>>>> -Sundar
>>>>
>>>>> Attila.
>>>>>
>>>>> On Aug 7, 2013, at 6:56 PM, A. Sundararajan 
>>>>> <sundararajan.athijegannathan at oracle.com> wrote:
>>>>>
>>>>>> Please review http://cr.openjdk.java.net/~sundar/8022524/
>>>>>>
>>>>>> Thanks
>>>>>> -Sundar
>