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

[Attila Szegedi]

- 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.

- 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().

- 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.

- 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.

- We still keep GlobalObject interface around?

- 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.

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