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

[A. Sundararajan]

Please review update webrev @ 
http://cr.openjdk.java.net/~sundar/8022524/webrev.01/

Additional changes from webrev.00:

* retain "source" private field in generated script classes (Jim noted 
that debugger needs to be able to get Source field from script Class 
objects).

* Removed OutOfMemoryError catching portion of script in 
test/script/basic/JDK-8020357.js - rather than just commenting out.

* Removed synchronization in RecompilableScriptFunctionData's ensureXYZ 
methods as suggested by Attila.

* Added a "revisit" comment to CompileUnit.java (to refactor to avoid 
non-final null-ed out fields in future -- as suggested by Attila).

* A bug in ScriptFunction.findCallMethod fixed - which was masked by 
megamorphic dynamic invoker method in UserAccessorProperty earlier. 
Tthis pre-existing issue surfaced after dynamic invokers were moved to 
global instance. One of the test262 tests ( 
ch10/10.4/10.4.3/10.4.3-1-105.js ) failed. Fixed by refactoring 
needsWrapperThis check out of needsCaller block.

PS. If Attila has further suggestions in response to my earlier emails, 
I'll incorporate and raise third round of webrev.

Please review.

Thanks
-Sundar


On Thursday 08 August 2013 09:15 AM, A. Sundararajan 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?
>
> 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
>>
>