Re: 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  
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

Re: RFR(S): 8149744: fix testng.jar delivery in Nashorn build.xml

[Hannes Wallnoefer]

+1


Am 2016-02-12 um 16:36 schrieb Michael Haupt:

Dear all,

please review this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8149744
Webrev: http://cr.openjdk.java.net/~mhaupt/8149744/webrev.00/

Nashorn's build.xml used to download a TestNG ZIP file and extract the required 
testng.jar from that. The URL of the ZIP file is no longer valid. The change 
addresses this by having build.xml download the four required JAR files (TestNG 
and dependencies).

Thanks,

Michael

Re: RFR(S): 8149744: fix testng.jar delivery in Nashorn build.xml

[Sundararajan Athijegannathan]

+1

On 2/12/2016 9:06 PM, Michael Haupt wrote:

Dear all,

please review this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8149744
Webrev: http://cr.openjdk.java.net/~mhaupt/8149744/webrev.00/

Nashorn's build.xml used to download a TestNG ZIP file and extract the required 
testng.jar from that. The URL of the ZIP file is no longer valid. The change 
addresses this by having build.xml download the four required JAR files (TestNG 
and dependencies).

Thanks,

Michael

RFR(S): 8149744: fix testng.jar delivery in Nashorn build.xml

[Michael Haupt]

Dear all,

please review this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8149744
Webrev: http://cr.openjdk.java.net/~mhaupt/8149744/webrev.00/

Nashorn's build.xml used to download a TestNG ZIP file and extract the required 
testng.jar from that. The URL of the ZIP file is no longer valid. The change 
addresses this by having build.xml download the four required JAR files (TestNG 
and dependencies).

Thanks,

Michael

-- 

 
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
  Oracle is committed to developing 
practices and products that help protect the environment

Re: Review request for JDK-8149451: Fix bytecode generation issue after 8149186

[Michael Haupt]

Hi Attila,

> Am 11.02.2016 um 15:38 schrieb Attila Szegedi :
> This is frankly maddening :-)


rightfully so, as it turns out this code is not the cause of the observed 
nightly failures. See https://bugs.openjdk.java.net/browse/JDK-8149727. The 
push for 8149186 merely coincided with the first nightly breaks. Sorry about 
the fuss.

So, consider this a lower-case thumbs-up review. :-)

Best,

Michael

-- 

 
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
  Oracle is committed to developing 
practices and products that help protect the environment