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
