Review request for 8022524 : Memory leaks in nashorn sources and tests found by jhat analysis
A. Sundararajan
sundararajan.athijegannathan at oracle.com
Thu Aug 8 05:38:33 PDT 2013
Thanks. As we discussed over skype, we had to do that (correctness) fix
in ScriptFunction.
PS. Perhaps we've to run test262 in separate context mode as well..
-Sundar
On Thursday 08 August 2013 04:45 PM, Hannes Wallnoefer wrote:
> +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