Review request for JDK-8147558: Add support for ES6 collections

[Michael Haupt]

Hi Hannes,

lower-case thumbs up.

Best,

Michael

> Am 12.02.2016 um 13:55 schrieb Hannes Wallnoefer <hannes.wallnoefer at 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.wallnoefer at 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