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

[A. Sundararajan]

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?

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?

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
>