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

[A. Sundararajan]

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.athijegannat...@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.athijegannat...@oracle.com> wrote:

Please review http://cr.openjdk.java.net/~sundar/8022524/

Thanks
-Sundar