Review request for 8022524 : Memory leaks in nashorn sources and tests found by jhat analysis
Hannes Wallnoefer
hannes.wallnoefer at oracle.com
Thu Aug 8 04:15:05 PDT 2013
+1.
I noticed some slight performance degradation in Richards and other
octane benchmarks that seems to be caused by the ScriptFunction fix. But
it after some in-depth checks there is no additional invalidation going
on, so it must be the slightly heavier guards which are actually
necessary to fix the object wrapping bug we already had.
Hannes
Am 2013-08-08 09:04, schrieb A. Sundararajan:
> enhanced :-)
>
> http://cr.openjdk.java.net/~sundar/8022524/webrev.03/
>
> -Sundar
>
> On Thursday 08 August 2013 11:58 AM, Attila Szegedi wrote:
>> +1 on your webrev.02;
>>
>> You could still enhance few things, such as:
>>
>> - you could just write the code for getInvokeByName and
>> getDynamicInvoker once if you wrote a generic version as private
>> static T getLazilyCreatedValue(final Object key, final Callable<T>
>> creator, final ConcurrentMap<Object, T>) (that's basically equivalent
>> to computeIfAbsent)
>> - you could inline the single-arg createIteratorCallbackInvoker into
>> the two-arg version, since it's no longer used from elsewhere
>>
>> but those are minor things.
>>
>> Attila.
>>
>>
>> On Aug 8, 2013, at 7:24 AM, A. Sundararajan
>> <sundararajan.athijegannathan at oracle.com> 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
>
More information about the nashorn-dev
mailing list