Hi Hannes, lower-case thumbs up.
Best, Michael > Am 12.02.2016 um 13:55 schrieb Hannes Wallnoefer > <hannes.wallnoe...@oracle.com>: > > Hi Attila, > > sorry for the interrupted conversation, I've been on vacation for most of > this week. > > Having had some time to think about the synchronization issue I've come to > agree with you. Making LinkedMap synchronized was overshooting the mark, and > basically meant protecting against wrong use of Nashorn. > > I've uploaded a new webrev that follows most of your suggestions: > > http://cr.openjdk.java.net/~hannesw/8147558/webrev.01/ > > I've decided to keep the NativeArray methods in their slightly redundant form > as I think the level of repetition is bearable and your distaste of it did > not seem too strong :) > > Since the patch is quite large I've also uploaded a patch with just the > changes from the first webrev: > > http://cr.openjdk.java.net/~hannesw/8147558/changes-attila > > Thanks, > Hannes > > > Am 2016-02-05 um 16:31 schrieb Attila Szegedi: >> On Feb 5, 2016, at 3:13 PM, Hannes Wallnoefer <hannes.wallnoe...@oracle.com> >> wrote: >>> Thanks for the review, Attila! >>> >>> Am 2016-02-05 um 14:17 schrieb Attila Szegedi: >>>> Global.java: >>>> - Documentation for Global.setWeakSet() is wrong. >>>> - It’s kinda too bad we can’t generalize those getters with lazy sentinel >>>> and builtin getters. I guess we could if they weren’t stored in fields. >>>> Sounds like a good future use case for a VarHandle :-) >>>> - Global.initPrototype uses the “jdk.nashorn.internal.objects." string >>>> literal. It’s already used by Global.initConstructor too, maybe extract it >>>> into a constant? (FWIW, it also appears without the trailing dot in >>>> NashornLoader.java; maybe unify it into a single place somewhere?) >>> Thanks, will fix the documentation and clean up the string literals. >>> >>>> NativeArray.java: >>>> - I’d refactor the common idiom in entries, keys, values, and getIterator >>>> into a separate method parametrized on iteration kind. FWIW, getIterator >>>> and values look exactly the same. >>> I'm not sure I understand. These are separate methods because each one >>> defines a @Function for nasgen. Of course we could extend the @Function >>> annotation to allow one Java method to define multiple JS functions. But >>> these methods are one-liners anyway, so I doubt it would get much simpler. >> private static Object createIterator(final Object self, final >> AbstractIterator.IterationKind kind) { >> return new ArrayIterator(Global.toObject(self), kind, Global.instance()); >> } >> >> then >> >> @Function(attributes = Attribute.NOT_ENUMERABLE) >> public static Object entries(final Object self) { >> return createIterator(AbstractIterator.IterationKind.KEY_VALUE); >> } >> >> etc. No big deal, but I like to eliminate all copy-paste code. >> >>>> LinkedMap.java: >>>> - why does it need to be safe for concurrent use? Seems unnecessary to me. >>> You're right, it's not a requirement, but I think it's nice to have given >>> the ubiquity of multithreading on the Java platform. >> I’m still in disagreement. For one argument, that makes this class closer to >> Java 1.1 Hashtable than to HashMap. The trend in collection classes is to >> not make them thread safe unless they’re specifically concurrent. For >> another argument, we also consciously don’t make JS classes thread safe >> since the JS language is not thread safe, so we don’t want to incur >> synchronization overhead in the majority usage single-threaded case. >> ScriptObject is not thread safe either. It’s a design principle we followed >> so far: JS objects homed in a single global are not thread safe; backing >> structures shared across globals in a context (compiled functions etc.) are >> thread safe. Having a class that violates this distinction can provide a >> misunderstanding in someone reading the code about these design principles. >> >> Of course, if the class would start to be used in a different context too, >> then it might be justified to add synchronization to it (or just have it >> implement java.util.Map and use Collections.synchronizedMap when this is >> desired). >> >>>> WeakMap and WeakSet: didn’t you need the guarantees of LinkedMap with them >>>> too? I guess not. >>> The weak collections do not allow elements to be iterated over, so this >>> isn't an issue there. >> Excellent. Thought there’s a reason :-) >> >>> Hannes > -- <http://www.oracle.com/> Dr. Michael Haupt | Principal Member of Technical Staff Phone: +49 331 200 7277 | Fax: +49 331 200 7561 Oracle Java Platform Group | LangTools Team | Nashorn Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher <http://www.oracle.com/commitment> Oracle is committed to developing practices and products that help protect the environment
