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

[Hannes Wallnoefer]

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