Updated based on comments from Attila.
Update webrev @ http://cr.openjdk.java.net/~sundar/8022524/webrev.02/
-Sundar
On Thursday 08 August 2013 10:54 AM, A. Sundararajan 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
<[email protected]> 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
<[email protected]> wrote:
Please review http://cr.openjdk.java.net/~sundar/8022524/
Thanks
-Sundar
