+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 <[email protected]> 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 >
