RFR: 8232209: Update double-conversion license file to version 3.1.5

[Hannes Wallnöfer]

Please review this trivial patch to update the version number in the 
double-conversion license file.

JBS: https://bugs.openjdk.java.net/browse/JDK-8232209
Webrev: http://cr.openjdk.java.net/~hannesw/8232209/webrev.00/

Thanks,
Hannes

Re: RFR: 8230709: Array index out of bounds in ES6 mode

[Hannes Wallnöfer]

Am 09.09.2019 um 16:50 schrieb Attila Szegedi :
> 
> +1. Ugh. I guess our default lexical context stack is 16 deep.
> 

It is indeed :) Thanks for reviewing.

Hannes


>> On 2019. Sep 9., at 16:38, Hannes Wallnöfer  
>> wrote:
>> 
>> Please review this off-by-one array index fix that was recently reported on 
>> this list:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8230709
>> Webrev: http://cr.openjdk.java.net/~hannesw/8230709/webrev.00/
>> 
>> Thanks,
>> Hannes
> 

RFR: 8230766: Changed message in IllegalMonitorStateException

[Hannes Wallnöfer]

Please review this fix for a test failing because of a changed exception 
message in java.lang.Object.wait():

JBS: https://bugs.openjdk.java.net/browse/JDK-8230766
Webrev: http://cr.openjdk.java.net/~hannesw/8230766/webrev.00/

Thanks,
Hannes

RFR: 8230709: Array index out of bounds in ES6 mode

[Hannes Wallnöfer]

Please review this off-by-one array index fix that was recently reported on 
this list:

JBS: https://bugs.openjdk.java.net/browse/JDK-8230709
Webrev: http://cr.openjdk.java.net/~hannesw/8230709/webrev.00/

Thanks,
Hannes

Re: ArrayIndexOutOfBoundsException in LexicalContext.java#inUnprotectedSwitchContext

[Hannes Wallnöfer]

Hi Anton,

Thanks for the report - that’s a really interesting one!

I’ll file a bug for it, but given that Nashorn is deprecated and this is part 
of the incomplete ES6 support I don’t think it will be deemed worthy of a 8u 
backport.

Hannes


> Am 09.08.2019 um 11:16 schrieb Anton Mitrofanov :
> 
> Hi.
> 
> We have encountered a bug in Nashorn with JDK8 u221. It can be reproduced by 
> evaluation of this script with "jjs --language=es6":
> 
> {{{ let x; }}}
> 
> It results in "java.lang.ArrayIndexOutOfBoundsException: 16" output. It need 
> exactly 15 curly braces to cause this bug.
> 
> And here is the patch to fix it:
> 
> diff -r 06eed83ab4cd 
> src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/LexicalContext.java
> --- 
> a/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/LexicalContext.java
>Tue Aug 06 12:14:41 20>
> +++ 
> b/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/LexicalContext.java
>Fri Aug 09 11:37:23 20>
> @@ -697,7 +697,7 @@
>  * @return {@code true} if in unprotected switch statement.
>  */
> public boolean inUnprotectedSwitchContext() {
> -for (int i = sp; i > 0; i--) {
> +for (int i = sp - 1; i > 0; i--) {
> final LexicalContextNode next = stack[i];
> if (next instanceof Block) {
> return stack[i - 1] instanceof SwitchNode;
> 
> P.S. Can we expect that this bug will be fixed in JDK8 updates?
> 

RFR: Update double-conversion to version 3.1.5

[Hannes Wallnöfer]

Please review the update of the Java port of the V8 double conversion library 
to the most recent release.

JBS: https://bugs.openjdk.java.net/browse/JDK-8227391
Webrev: http://cr.openjdk.java.net/~hannesw/8227391/webrev.00/

Although the commit log between the two releases is very long [1] (I’m not sure 
the list is totally accurate since they are on different branches) the actual 
changes to the code are quite limited. The majority of changes either pertains 
to the code layout, build system, or are C++ specific and do not apply to the 
Java port. In the end it amounts to two fixes plus tests along with some code 
cleanup.

[1]: https://github.com/google/double-conversion/compare/57b1e09...5fa81e8

This will go into 13 or 14, depending on whether the late enhancement is 
allowed.

Thanks,
Hannes

Re: Nashorn on the module-path

[Hannes Wallnöfer]

Hi Christian,

I cloned and tried your example project. When I run the project, I get one 
successful and one aborted tests in both cases:

Module path output:

└─ JUnit Jupiter ✔
   └─ CheckTests ✔
  ├─ test() ✔
  └─ emitStringRepresentationOfTestModule() ■ Assumption failed: module 
check

Class path output:

└─ JUnit Jupiter ✔
   └─ CheckTests ✔
  ├─ test() ✔
  └─ emitStringRepresentationOfTestModule() ■ Assumption failed: unnamed 
module @306f16f3


Unfortunately it’s quite hard for me to understand what’s going on from there. 
Your Main class is quite complex with over 800 lines of code, and it seems like 
the interesting code is in junit-jupiter, which is included in binary form only.

Do you think it’s possible to reduce the problem further, ideally to a plain 
java class?


Hannes


> Am 27.05.2019 um 11:40 schrieb Christian Stein :
> 
> On Mon, May 27, 2019 at 11:37 AM Sundararajan Athijegannathan <
> sundararajan.athijegannat...@oracle.com> wrote:
> 
>> How can this be reproduced at out end?
> 
> 
> I compiled a small example project at [1] that describes and
> demonstrates the issue. Please view the README.md file for
> details.
> 
> You may reproduce the issue by launching `jshell build.jsh`
> within the root directory of the project -- having JDK 11+ installed.
> 
> [1] https://github.com/sormuras/junit5-class-vs-module-path

RFR: 8222528: Fix javadoc headers in Nashorn sources

[Hannes Wallnöfer]

Please review:

Issue: https://bugs.openjdk.java.net/browse/JDK-8222528
Webrev: http://cr.openjdk.java.net/~hannesw/8222528/webrev.00/

Javadoc now requires that headings be consistent with accessibility guidelines. 
Since h1 is usually the page heading user documentation headings should start 
with h2.

Thanks,
Hannes

RFR: 8214795: Add safety check to dynalink inner class lookup

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8214795
Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/

This is to make sure we use the right inner classes regardless of the order of 
classes returned by Class.getClasses().

Thanks,
Hannes

Re: RFR: 8210943: Hiding of inner classes not resolved properly

[Hannes Wallnöfer]

Attila, the subclass-to-superclass traversal is actually not done in dynalink 
but directly in java.lang.Class.getClasses(). So Sundar has a point in that my 
code relies on implementation rather than specified behaviour of 
Class.getClasses().

Now I do think it is highly unlikely that the order of classes returned by 
Classes.getClasses() would change in a future release. That would certainly 
break a lot of other code. Furthermore, if the order was reversed (superclasses 
to subclasses) that change would be caught by the test. The only change that 
could go unnoticed would be classes returned in random order, which I don’t 
think is a real concern. But of course we could choose to be defensive and add 
code that guards against it.

Hannes


> Am 01.12.2018 um 13:05 schrieb Attila Szegedi :
> 
> The relevant Dynalink algorithm processes the class before moving on to 
> superclass, so Hannes fix is correct in that we won’t stomp over a subclass’ 
> inner class (inserted into the map earlier) with the superclass’ inner class 
> (encountered later by the algorithm). So yeah, getClasses() doesn’t specify 
> an order, but the Dynalink code has a subclass-towards-superclass traversal 
> order.
> 
> Attila.
> 
>> On 2018. Dec 1., at 7:13, Sundararajan Athijegannathan 
>>  wrote:
>> 
>> Class.getClasses() javadoc does not mention anything about order of classes 
>> returned.
>> 
>> https://docs.oracle.com/javase/10/docs/api/java/lang/Class.html#getClasses()
>> 
>> Do we need a check using Class.getDeclaringClass() or do I something here?
>> 
>> Thanks,
>> -Sundar
>> 
>> On 30/11/18, 4:44 PM, Attila Szegedi wrote:
>>> +1. Thanks for fixing this.
>>> 
>>>> On 2018. Nov 29., at 18:01, Hannes Wallnöfer 
>>>>  wrote:
>>>> 
>>>> Please review:
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210943
>>>> Webrev: http://cr.openjdk.java.net/~hannesw/8210943/webrev.00/
>>>> 
>>>> AccessibleMembersLookup#lookupAccessibleMembers adds all nested classes 
>>>> returned by Class.getClasses(), but these may contain inherited classes 
>>>> that are shadowed and thus not visible from the current class. The 
>>>> solution is to make sure we use the first inner class with any given name.
>>>> 
>>>> Thanks,
>>>> Hannes
> 

Re: RFR: 8214525: Bit rot in Nashorn Ant script

[Hannes Wallnöfer]

Am 30.11.2018 um 14:04 schrieb Jim Laskey :
> 
> +1
> 
> too bad you have to hardcode the version

12 docs aren’t in their final place, and who knows if they’ll change the URL 
scheme again :)

Thanks!

Hannes

>> On Nov 30, 2018, at 6:58 AM, Hannes Wallnöfer  
>> wrote:
>> 
>> Please review: 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214525
>> Webrev: http://cr.openjdk.java.net/~hannesw/8214525/webrev.00/
>> 
>> This enables gzip encoding for the YUI download and switches javadoc tasks 
>> to modularized version of -linkoffline.
>> 
>> Thanks,
>> Hannes
> 

RFR: 8214525: Bit rot in Nashorn Ant script

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8214525
Webrev: http://cr.openjdk.java.net/~hannesw/8214525/webrev.00/

This enables gzip encoding for the YUI download and switches javadoc tasks to 
modularized version of -linkoffline.

Thanks,
Hannes

RFR: 8210943: Hiding of inner classes not resolved properly

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210943
Webrev: http://cr.openjdk.java.net/~hannesw/8210943/webrev.00/

AccessibleMembersLookup#lookupAccessibleMembers adds all nested classes 
returned by Class.getClasses(), but these may contain inherited classes that 
are shadowed and thus not visible from the current class. The solution is to 
make sure we use the first inner class with any given name.

Thanks,
Hannes

Re: casting typed array to java byte[] is it possible?

[Hannes Wallnöfer]

Hi Paulo,

Java.to() would be the way to go, but as you found out it does not support 
typed arrays.

What works is to convert the typed array to an ordinary JS array and convert to 
byte[] from there:

  Java.to(Array.prototype.slice.call(arr), 'byte[]‘);

That’s obviously not very elegant nor efficient, but it’s the only workaround I 
can think of.

Hannes


> Am 11.10.2018 um 20:21 schrieb Paulo Lopes :
> 
> Hi,
> 
> I'm trying to handle a case where a Uint8Array is being passed to a
> method, that has the signature:
> 
> String encode(byte[]);
> 
> Sadly nashorn fails with:
> 
> java.lang.ClassCastException: Cannot cast
> jdk.nashorn.internal.objects.NativeUint8Array to [B
> 
> And trying to help the cast with:
> 
> javaObj.encode(Java.to(arr, 'byte[]'));
> 
> Does not help either. The documentation on typed arrays is quite scarce
> so I cannot see how to handle this, does anyone have a clue?
> 
> Thanks!
> Paulo
> 

Re: RFR 8204492 Add deprecation annotation to Nashorn APIs and warning to nashorn, jjs

[Hannes Wallnöfer]

Looks good.

Hannes

> Am 27.06.2018 um 06:19 schrieb Sundararajan Athijegannathan 
> :
> 
> Forgot to CC build-dev for makefile changes.
> 
> -Sundar
> 
> On 27/06/18, 9:46 AM, Sundararajan Athijegannathan wrote:
>> Please review.
>> 
>> Bug https://bugs.openjdk.java.net/browse/JDK-8204492
>> Webrev http://cr.openjdk.java.net/~sundar/8204492/webrev.01
>> 
>> Related:
>> 
>> JEP http://openjdk.java.net/jeps/335
>> CSR https://bugs.openjdk.java.net/browse/JDK-8205594
>> 
>> Thanks,
>> -Sundar
>> 

Re: RFR: 8203814: javac --release=8 \"cannot find symbol\" for NashornException.getEcmaError()

[Hannes Wallnöfer]

Hi Jan,

The changes for Nashorn symbols all look good to me.

Thanks,
Hannes

> Am 19.06.2018 um 13:14 schrieb Jan Lahoda :
> 
> Hi Hannes,
> 
> Thanks for the comment, updated webrev:
> http://cr.openjdk.java.net/~jlahoda/8203814/webrev.01/
> 
> (It also includes a few more tweaks that turned out to be needed.)
> 
> Jan
> 
> On 15.6.2018 16:52, Hannes Wallnöfer wrote:
>> Thanks for helping out with this, Jan.
>> 
>> Unfortunately I found another change that was backported to 8u102 that 
>> changes the signatures of ScriptUtils makeSynchronizedFunction(…) and 
>> wrap(…) methods (again).
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8148379
>> 
>> I’m sorry about this, I should have checked when you first asked me about it.
>> 
>> Otherwise the patch looks good to me.
>> 
>> Hannes
>> 
>> 
>>> Am 15.06.2018 um 15:47 schrieb Jan Lahoda :
>>> 
>>> Hi,
>>> 
>>> The Nashorn APIs have been enhanced in JDK 8 updates. The proposal here is 
>>> to update the historical data for --release 8 to JDK 8u40.
>>> 
>>> Note this includes removal of:
>>> jdk.nashorn.api.scripting.NashornException.ENGINE_SCRIPT_SOURCE_NAME
>>> and:
>>> jdk.nashorn.api.scripting.NashornScriptEngine.__noSuchProperty__(...)
>>> and a change of:
>>> jdk.nashorn.api.scripting.ScriptUtils.wrap(...)
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203814
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8203814/webrev.00/
>>> 
>>> How does this look?
>>> 
>>> Thanks,
>>>   Jan
>> 

Re: RFR: 8203814: javac --release=8 \"cannot find symbol\" for NashornException.getEcmaError()

[Hannes Wallnöfer]

Thanks for helping out with this, Jan.

Unfortunately I found another change that was backported to 8u102 that changes 
the signatures of ScriptUtils makeSynchronizedFunction(…) and wrap(…) methods 
(again).

https://bugs.openjdk.java.net/browse/JDK-8148379

I’m sorry about this, I should have checked when you first asked me about it.

Otherwise the patch looks good to me.

Hannes


> Am 15.06.2018 um 15:47 schrieb Jan Lahoda :
> 
> Hi,
> 
> The Nashorn APIs have been enhanced in JDK 8 updates. The proposal here is to 
> update the historical data for --release 8 to JDK 8u40.
> 
> Note this includes removal of:
> jdk.nashorn.api.scripting.NashornException.ENGINE_SCRIPT_SOURCE_NAME
> and:
> jdk.nashorn.api.scripting.NashornScriptEngine.__noSuchProperty__(...)
> and a change of:
> jdk.nashorn.api.scripting.ScriptUtils.wrap(...)
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8203814
> Webrev: http://cr.openjdk.java.net/~jlahoda/8203814/webrev.00/
> 
> How does this look?
> 
> Thanks,
>   Jan

RFR: 8204290: Add check to limit number of capture groups

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8204290
Webrev: http://cr.openjdk.java.net/~hannesw/8204290/webrev/

This (like the previous RFR) is another backport from jruby/joni.

Thanks,
Hannes

RFR: 8204288: Matching the end of a string followed by an empty greedy regex and a word boundary fails

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8204288
Webrev: http://cr.openjdk.java.net/~hannesw/8204288/webrev/

Thanks,
Hannes

Re: RFR: JDK-8203827: Upgrade JLine to 2.14.6

[Hannes Wallnöfer]

Hi Jan, 

Works as expected with jjs now.

Thanks,
Hannes

> Am 30.05.2018 um 17:06 schrieb Jan Lahoda :
> 
> Hi,
> 
> An updated webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.01/complete/
> 
> A webrev showing changes from the previous revision is here:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.01/delta/
> 
> The changes are:
> -updated src/jdk.internal.le/share/legal/jline.md
> -the problem with automatically adding space after completion is resolved
> -a few tweaks to tests
> 
> Does this look good?
> 
> Thanks,
>Jan
> 
> On 29.5.2018 16:04, Jan Lahoda wrote:
>> Hi,
>> 
>> On 29.5.2018 14:51, Alan Bateman wrote:
>>> On 25/05/2018 21:20, Jan Lahoda wrote:
 Hi,
 
 I'd like to upgrade the JLine used by JShell and jjs from 2.12.1 to
 2.14.6.
 
 The complete webrev is here:
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/complete/
 
 To simplify reviewing, there is:
 -an antipatch that removes the JDK-specific changes and restores the
 vanilla 2.12.1 content:
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/undo-jdk-extras/
 -a patch that replaces the 2.12.1 content with 2.14.6:
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/upgrade-jline/
 -a patch that re-applies the JDK-specific changes (like including
 adjusting packages, and removal/commenting out of usage of features
 that would require undesirable dependencies, and any changes that had
 to be done to other modules):
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/adding-jdk-extras/
 
 JBS entry: https://bugs.openjdk.java.net/browse/JDK-8203827
 
 How does this look?
>>> The refresh looks okay and I see the package name has been adjusted for
>>> the new files.
>> 
>> Thanks.
>> 
>>> 
>>> Is there an update to src/jdk.internal.le/share/legal/jlink.md?
>>> Minimally I would assume the version at the top of the file needs to be
>>> updated.
>> 
>> Yes, it should, I forgot to do that. I'll send an update later.
>> 
>> Jan
>> 
>>> 
>>> -Alan

Re: RFR: JDK-8203827: Upgrade JLine to 2.14.6

[Hannes Wallnöfer]

Hi Jan,

Nashorn changes look good.

I noticed one slight change of behaviour in jjs. When I enter „java.m“ and hit 
the tab key, it autocompletes to „java.math “, adding a space character at the 
end. This is a bit inconvenient, and the old version of jline didn’t do that.

Hannes


> Am 25.05.2018 um 22:20 schrieb Jan Lahoda :
> 
> Hi,
> 
> I'd like to upgrade the JLine used by JShell and jjs from 2.12.1 to 2.14.6.
> 
> The complete webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/complete/
> 
> To simplify reviewing, there is:
> -an antipatch that removes the JDK-specific changes and restores the vanilla 
> 2.12.1 content:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/undo-jdk-extras/
> -a patch that replaces the 2.12.1 content with 2.14.6:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/upgrade-jline/
> -a patch that re-applies the JDK-specific changes (like including adjusting 
> packages, and removal/commenting out of usage of features that would require 
> undesirable dependencies, and any changes that had to be done to other 
> modules):
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/adding-jdk-extras/
> 
> JBS entry: https://bugs.openjdk.java.net/browse/JDK-8203827
> 
> How does this look?
> 
> Thanks,
>Jan

RFR: 8200716: Object propertyIsEnumerable buggy behavior on short integer-string key

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8200716
Webrev: http://cr.openjdk.java.net/~hannesw/8200716/webrev/

Thanks, 
Hannes

Re: Longs aren't numbers in Java 8u162

[Hannes Wallnöfer]

Hi Ryan,

Yes, this change was intentional. We were aware at the time that it would cause 
some problems, but on the other hand treating longs as numbers clashed with the 
ECMA spec and caused silent loss of precision. 

In most cases there should be simple workarounds, like using the unary + 
operator to convert an object to a number. Sorry for the inconvenience this has 
been causing!

Hannes


> Am 01.05.2018 um 00:38 schrieb Ryan Berdeen :
> 
> I encountered an issue when upgrading from Java 8u66 to 8u162. In 8u66,
> java.lang.Integer and java.lang.Long both behaved as JS numbers. In 8u162
> (and 10.0.1), Longs behave differently in a way that is breaking my
> application.
> 
>function test(n) {
>  print(n);
>  print(typeof n);
>  print(n.hasOwnProperty('x'));
>  print();
>}
> 
>test(new java.lang.Integer(0));
>test(new java.lang.Long(0));
> 
> In 8u66, this behaved the same for Integer and Long, but in 8u162, typeof
> returns "object", and the object doesn't have the expected properties:
> 
>$ jjs longs.js
>0
>number
>false
> 
>0
>object
>longs.js:4 TypeError: n.hasOwnProperty is not a function
> 
> In my application, this came up with values passed through to the engine in
> bindings.
> 
> Is this change intentional? If so, is there a way to revert to the previous
> behavior and if not, is there a way to work around it so that I can update?

RFR: 8198816: AbstractScriptEngine.getScriptContext creation of SimpleScriptContext is inefficient

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8198816
Webrev: http://cr.openjdk.java.net/~hannesw/8198816/webrev/

Thanks,
Hannes

RFR: 8201466: Nashorn: defineProperty setters/getters on prototype object ignored with numeric property names

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8201466
Webrev: http://cr.openjdk.java.net/~hannesw/8201466/webrev/

Thanks,
Hannes

Re: Cast/boxing/unboxing of numbers Java 9

[Hannes Wallnöfer]

Disabling optimistic-types will make it behave like JDK 8 and shouldn’t have 
any side effects. In fact, you can make JDK 8 behave like JDK 9 by passing 
-ot=true.  

Hannes


> Am 27.03.2018 um 19:56 schrieb Paulo Oliveira <paulo.olive...@ifood.com.br>:
> 
> Hello,
> 
> Firstly, thanks for the quick answer.
> I tried some parameters however did not work, I suppose I miss typed 
> something.
> Disabling optimistic-types will work just like JDK 8 or may have some other 
> side effect  ?
> 
> 
> Em ter, 27 de mar de 2018 às 12:51, Hannes Wallnöfer 
> <hannes.wallnoe...@oracle.com> escreveu:
> Hi Paulo,
> 
> What you are observing is a side effect of the optimistic-types feature, 
> which was shipped with JDK 8u, but only became enabled by default with JDK 9. 
> It is unfortunate that this affects observable behaviour in this case, but 
> conversion between JavaScript and Java types is quite difficult and it’s hard 
> to avoid border cases like this.
> 
> As a workaround you can disable this feature using the nashorn.args system 
> property:
> 
> java -Dnashorn.args=-ot=false
> 
> Hannes
> 
> 
> > Am 26.03.2018 um 23:27 schrieb Paulo Oliveira <paulo.olive...@ifood.com.br>:
> >
> > Hello,
> >
> > Testing some nashorn scripts on java 9 I got some unexpected behavior.
> > The code:
> >
> > import java.util.Arrays;
> > import java.util.List;
> > import javax.script.Invocable;
> > import javax.script.ScriptEngine;
> > import javax.script.ScriptEngineManager;
> >
> > public class Main {
> >
> >public static void main(String[] args) throws Exception{
> >ScriptEngine scriptEngine = new
> > ScriptEngineManager().getEngineByName("Nashorn");
> >scriptEngine.eval("var imports = new JavaImporter(java.lang.Integer, 
> > " +
> >"java.lang.Long);" +
> >"var fun1 = function(longs, ints, id) {" +
> >"with(imports) {" +
> >"print(id.getId().class);" +
> >"print(longs.contains(id.getId()));" +
> >"print(ints.contains(id.getId()));" +
> >"return Long.valueOf(1);" +
> >"}" +
> >"};");
> >Invocable invocable = (Invocable) scriptEngine;
> >
> >List longs = Arrays.asList(1L, 2L);
> >List ints = Arrays.asList(1, 2);
> >Teste teste = new Teste(1L);
> >Object result = invocable.invokeFunction("fun1", longs, ints, teste);
> >System.out.println(result.getClass());
> >}
> > }
> >
> > public class Teste {
> >
> >
> >private Long id;
> >
> >public Teste(Long id) {
> >this.id =id;
> >}
> >
> >public Long getId() {
> >return id;
> >}
> > }
> >
> >
> > Executing the code
> > It produces:
> >
> > Java 8.161
> > class java.lang.Long
> > true
> > false
> > class java.lang.Long
> >
> > Java 9.0.4 and 10
> > class java.lang.Long
> > false
> > true
> > class java.lang.Integer
> >
> > Nashorn is changing every number that fits a Integer in Integers, even if I
> > explicit define as Long.
> > This causes a problem on List.contains, because it receives a Object, if I
> > try to verify if a long exists in a list of longs, it returns false.
> >
> > Is it a expected behavior ?
> > --
> > *Paulo Oliveira*
> > *Desenvolvedor BackEnd*
> >
> > *TEL:* +55 (11)  3634-3378
> > www.ifood.com.br
> >
> >
> >
> >
> > <https://itunes.apple.com/br/app/ifood-delivery-e-entrega-comida/id483017239?mt=8>
> > <https://play.google.com/store/apps/details?id=br.com.brainweb.ifood>
> > <https://www.facebook.com/iFood?fref=ts> <https://twitter.com/iFood>
> 
> -- 
> Paulo Oliveira
> Desenvolvedor BackEnd
> 
> TEL: +55 (11)  3634-3378
> www.ifood.com.br
> 
>
>   
>

Re: Cast/boxing/unboxing of numbers Java 9

[Hannes Wallnöfer]

Hi Paulo,

What you are observing is a side effect of the optimistic-types feature, which 
was shipped with JDK 8u, but only became enabled by default with JDK 9. It is 
unfortunate that this affects observable behaviour in this case, but conversion 
between JavaScript and Java types is quite difficult and it’s hard to avoid 
border cases like this.

As a workaround you can disable this feature using the nashorn.args system 
property:

java -Dnashorn.args=-ot=false

Hannes


> Am 26.03.2018 um 23:27 schrieb Paulo Oliveira :
> 
> Hello,
> 
> Testing some nashorn scripts on java 9 I got some unexpected behavior.
> The code:
> 
> import java.util.Arrays;
> import java.util.List;
> import javax.script.Invocable;
> import javax.script.ScriptEngine;
> import javax.script.ScriptEngineManager;
> 
> public class Main {
> 
>public static void main(String[] args) throws Exception{
>ScriptEngine scriptEngine = new
> ScriptEngineManager().getEngineByName("Nashorn");
>scriptEngine.eval("var imports = new JavaImporter(java.lang.Integer, " 
> +
>"java.lang.Long);" +
>"var fun1 = function(longs, ints, id) {" +
>"with(imports) {" +
>"print(id.getId().class);" +
>"print(longs.contains(id.getId()));" +
>"print(ints.contains(id.getId()));" +
>"return Long.valueOf(1);" +
>"}" +
>"};");
>Invocable invocable = (Invocable) scriptEngine;
> 
>List longs = Arrays.asList(1L, 2L);
>List ints = Arrays.asList(1, 2);
>Teste teste = new Teste(1L);
>Object result = invocable.invokeFunction("fun1", longs, ints, teste);
>System.out.println(result.getClass());
>}
> }
> 
> public class Teste {
> 
> 
>private Long id;
> 
>public Teste(Long id) {
>this.id =id;
>}
> 
>public Long getId() {
>return id;
>}
> }
> 
> 
> Executing the code
> It produces:
> 
> Java 8.161
> class java.lang.Long
> true
> false
> class java.lang.Long
> 
> Java 9.0.4 and 10
> class java.lang.Long
> false
> true
> class java.lang.Integer
> 
> Nashorn is changing every number that fits a Integer in Integers, even if I
> explicit define as Long.
> This causes a problem on List.contains, because it receives a Object, if I
> try to verify if a long exists in a list of longs, it returns false.
> 
> Is it a expected behavior ?
> -- 
> *Paulo Oliveira*
> *Desenvolvedor BackEnd*
> 
> *TEL:* +55 (11)  3634-3378
> www.ifood.com.br
> 
> 
> 
> 
> 
> 
>  

Re: RFR 8200215: 17th loop of "let foo = ''"; throws ReferenceError

[Hannes Wallnöfer]

+1

Hannes


> Am 26.03.2018 um 15:27 schrieb Sundararajan Athijegannathan 
> <sundararajan.athijegannat...@oracle.com>:
> 
> Updated: http://cr.openjdk.java.net/~sundar/8200215/webrev.01/
> 
> Please review.
> 
> -Sundar
> 
> On 26/03/18, 6:03 PM, Hannes Wallnöfer wrote:
>> The new ScriptObject.declareAndSet method is almost identical, so the 
>> existing one could be rewritten to make use of the new one.
>> 
>> Otherwise looks good.
>> 
>> Hannes
>> 
>> 
>>> Am 26.03.2018 um 14:00 schrieb Sundararajan 
>>> Athijegannathan<sundararajan.athijegannat...@oracle.com>:
>>> 
>>> Please review http://cr.openjdk.java.net/~sundar/8200215/webrev.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8200215
>>> 
>>> Thanks,
>>> -Sundar

Re: RFR 8200215: 17th loop of "let foo = ''"; throws ReferenceError

[Hannes Wallnöfer]

The new ScriptObject.declareAndSet method is almost identical, so the existing 
one could be rewritten to make use of the new one. 

Otherwise looks good.

Hannes


> Am 26.03.2018 um 14:00 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review http://cr.openjdk.java.net/~sundar/8200215/webrev.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8200215
> 
> Thanks,
> -Sundar

RFR: 8199869: Missing copyright headers in nashorn source code

[Hannes Wallnöfer]

Please review (trivial change, two copyright headers added):

Bug: https://bugs.openjdk.java.net/browse/JDK-8199869
Webrev: http://cr.openjdk.java.net/~hannesw/8199869/webrev/

Thanks,
Hannes

RFR: 8199443: Nashorn multithread bottleneck with "use strict"

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8199443
Webrev: http://cr.openjdk.java.net/~hannesw/8199443/webrev/

If we make sure the property maps for strict functions and arguments are set up 
correctly (they weren’t before) we can skip setting the property in 
ScriptObject#initUserAccessors.

Thanks,
Hannes

RFR: 8199236: Nashorn uses deprecated HTML tags in Javadoc

[Hannes Wallnöfer]

Please review 8199236: Nashorn uses deprecated HTML tags in Javadoc:

Bug: https://bugs.openjdk.java.net/browse/JDK-8199236
Webrev: http://cr.openjdk.java.net/~hannesw/8199236/webrev/

Thanks,
Hannes

Re: Da Capo: JSON "clear text" signatures

[Hannes Wallnöfer]

Thanks for the clarification, Anders.  

If you are working from Nashorn, the simplest way to enforce JS formatting is 
to coerce the Java number (whatever type it is) to a JS number, e.g. using 
unary + operator. 

If you need this in Java core libs, the place to discuss this would  be the 
core-libs-dev mailing list rather than nashorn-dev, but I think the chances to 
get such specialised functionality to core libs would be very slim.

Hannes

> Am 29.01.2018 um 15:46 schrieb Anders Rundgren 
> <anders.rundgren@gmail.com>:
> 
> On 2018-01-29 15:49, Sundararajan Athijegannathan wrote:
>> Just to be clear. so.. do you want a toString (static method?) variant
>> in java.lang.Double class as per this specification?
> 
> I don't think this is a major issue because all fundamental JSON types anyway 
> needs be dealt with separately or through a super interface.
> The existing Nashorn method (after upgrade to match ES better) moved to the 
> Java layer would probably be just fine.
> 
> Anders
> 
>> -Sundar
>> On 29/01/18, 7:45 PM, Anders Rundgren wrote:
>>> On 2018-01-29 14:52, Hannes Wallnöfer wrote:
>>>> Hi Anders,
>>>> 
>>>> I think I lack the context required to understand what you’re asking
>>>> for. Can you explain how transmitting numbers/doubles in JSON should
>>>> work and how the static method you’re asking for would enable this?
>>> 
>>> Sure.  Signatures depend on that data appears identical on both sides
>>> (sender + receiver).
>>> If one side outputs the integer 10 as 10.0 (which is OK JSON-wise),
>>> the signature will break in an EcmaScript environment where it must be
>>> 10 and nothing else.
>>> JSON tools would call the proposed static method rather than building
>>> their own number serializer.
>>> 
>>> Initially I thought number serialization was a simple problem but that
>>> was entirely wrong :-)
>>> Fortunately the ECMA folks have the expertize needed and their
>>> solution is already supported in billions of devices.
>>> Nashorn almost cuts it but only for JavaScript, not Java.
>>> 
>>>> Also, is there a document somewhere describing the IETF
>>>> standardization work you’re talking about?
>>> 
>>> You will have to wait to next week (when it becomes public), but in
>>> the meantime you can take a look at the core "input specifications":
>>> https://cyberphone.github.io/doc/security/jose-jcs.html
>>> https://cyberphone.github.io/doc/security/jose-jef.html
>>> 
>>> Thanx,
>>> Anders
>>> 
>>>> 
>>>> Thanks,
>>>> Hannes
>>>> 
>>>>> Am 28.01.2018 um 10:48 schrieb Anders Rundgren
>>>>> <anders.rundgren@gmail.com>:
>>>>> 
>>>>> The JSON "clear text" signature initiative seems to (finally) be
>>>>> headed for IETF standardization.  The plan is having a BOF session
>>>>> at the next IETF in London.
>>>>> 
>>>>> This scheme builds on EcmaScript JSON processing rules for data
>>>>> normalization which only rely on JSON.parse() and JSON.stringify().
>>>>> 
>>>>> A thorny issue for implementers is though serializing the JSON
>>>>> "Number" type.
>>>>> 
>>>>> An with Node.js, Chrome, Firefox, Safari (unfortunately not entirely
>>>>> compatible...) solution is currently available in "Nashorn":
>>>>> http://hg.openjdk.java.net/jdk8/jdk8/nashorn/file/096dc407d310/src/jdk/nashorn/internal/objects/NativeNumber.java
>>>>> 
>>>>> 
>>>>> It would be great if such support could for example be included as a
>>>>> static method in java.lang.Double, making Java and
>>>>> EcmaScript/JavaScript 100% interoperable with respect to this
>>>>> feature, the rest is actually close to trivial.
>>>>> 
>>>>> thanx,
>>>>> Anders
>>>>> https://github.com/OAI/OpenAPI-Specification/issues/1464#issue-291622705
>>>>> 
>>>> 
>>> 
> 

Re: Da Capo: JSON "clear text" signatures

[Hannes Wallnöfer]

Hi Anders,

I think I lack the context required to understand what you’re asking for. Can 
you explain how transmitting numbers/doubles in JSON should work and how the 
static method you’re asking for would enable this?

Also, is there a document somewhere describing the IETF standardization work 
you’re talking about?

Thanks,
Hannes

> Am 28.01.2018 um 10:48 schrieb Anders Rundgren 
> :
> 
> The JSON "clear text" signature initiative seems to (finally) be headed for 
> IETF standardization.  The plan is having a BOF session at the next IETF in 
> London.
> 
> This scheme builds on EcmaScript JSON processing rules for data normalization 
> which only rely on JSON.parse() and JSON.stringify().
> 
> A thorny issue for implementers is though serializing the JSON "Number" type.
> 
> An with Node.js, Chrome, Firefox, Safari (unfortunately not entirely 
> compatible...) solution is currently available in "Nashorn":
> http://hg.openjdk.java.net/jdk8/jdk8/nashorn/file/096dc407d310/src/jdk/nashorn/internal/objects/NativeNumber.java
> 
> It would be great if such support could for example be included as a static 
> method in java.lang.Double, making Java and EcmaScript/JavaScript 100% 
> interoperable with respect to this feature, the rest is actually close to 
> trivial.
> 
> thanx,
> Anders
> https://github.com/OAI/OpenAPI-Specification/issues/1464#issue-291622705

Re: RFR: 8147614: add jjs test for -t option

[Hannes Wallnöfer]

+1

Hannes

> Am 23.01.2018 um 07:34 schrieb Priya Lakshmi Muthuswamy 
> :
> 
> Hi,
> 
> Kindly review JDK-8147614: add jjs test for -t option
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8147614
> webrev: http://cr.openjdk.java.net/~pmuthuswamy/8147614/webrev.00/
> 
> Thanks,
> Priya

Re: RFR 8195829 Parsing a nameless ES6 class results in a thrown NullPointerException.

[Hannes Wallnöfer]

+1

Hannes

> Am 22.01.2018 um 15:44 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Webrev: http://cr.openjdk.java.net/~sundar/8195829/webrev.00/index.html
> Bug: https://bugs.openjdk.java.net/browse/JDK-8195829
> 
> Thanks,
> -Sundar

Re: ES6 and JavaImporter

[Hannes Wallnöfer]

Brad,

I as I wrote in my last reply to this list, ES6 support is quite incomplete in 
Nashorn.

However, that doesn’t really explain this error. I’m looking into it and will 
file a bug.

Hannes

> Am 19.01.2018 um 04:06 schrieb Brad Grier :
> 
> Hello,
> 
> I wanted to experiment with the Java 9 ES6 features in my software.
> Unfortunately I can't get far because of an issue involving JavaImporter. I
> created a simple jjs example that illustrates the problem. The script works
> under ES5 but fails under ES6 with:  ./pg4.js:6 ReferenceError: "printIt"
> is not defined
> 
> #!/usr/bin/jjs -J-Dnashorn.args=--language=es6
> 
> var imports = new JavaImporter(java.util);
> with(imports){
> 
>function printIt(a){
>print("map: " + a);
>}
> 
>function printAndClear(b){
>printIt(b);
>b.clear();
>}
> 
>var map = new HashMap();
>map.put('js', 'javascript');
>map.put("java", "java");
>printAndClear(map);
> }
> 
> Thanks. I'm using build 9.0.1+11 on OS X.
> 
> Brad

Re: ES6 from Java, using Nashorn

[Hannes Wallnöfer]

Hi Olivier,

Support for ECMAScript 6 is quite incomplete in Nashorn unfortunately. We only 
support a small set of ES6 features in JDK 9:

• Template strings
• let, const, and block scope
• Iterators and for..of loops
• Map, Set, WeakMap, and WeakSet
• Symbols
• Binary and octal literals

„require“ on the other hand is not part of ES6. It was created as a way to 
implement modules in the pre-ES6 world. It can be implemented in script and 
there are implementations out there, so this is probably the most promising 
approach to use modules in Nashorn.

Hannes

> Am 08.01.2018 um 17:33 schrieb Olivier :
> 
> Hi experts,
> I want to run ES6 scripts from Java, using Nashorn.
> I have scripts involving 'let', 'const', my Java code is happy with it, I 
> just need to use a property "nashorn.args" set to "--language=es6".
> 
> Now, I'd like to move forward, use imports and modules.
> I have one script like this (modules.01.js):
> 
> function displayMessage() {
> console.log("Hello JS World!");
> };
> 
> export default displayMessage;
> 
> I want to consume it from this (modules.consume.js):
> 
> import displayMessagefrom './modules.01';
> 
> displayMessage();
> 
> But this is where I have a question:
> My engine.eval("load('./modules.consume.js');"); complains about the "import" 
> statement.
> In the transpiled version, it complains about the "require" statement.
> 
> I must be missing something... I attach my code, in case it's needed.
> 
> Any idea, pointer, comment, help, etc, most welcome,
> Thank you!
> - Olivier
> 
> 
> 
> 

RFR: 8195123: Very large regressions in Octane benchmarks using 10-b39

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8195123
Webrev: http://cr.openjdk.java.net/~hannesw/8195123/webrev.00/

This undoes the recent fix for JDK-8193567 (Attila, in case you’re reading, I 
should have taken your doubts more serously) and replaces it with a different 
fix.

Instead of disabling optimistic types within comparison nodes, the new fix  
avoids the shortcuts for null and undefined comparison if the compared 
expression contains an optimistic expression. Previously we only checked 
whether the expression itself was optimistic or contained the expression that 
triggered the current rest-of compilation. It’s easy to see why we must avoid 
it also for nested optimistic expressions, as they can trigger deoptimization 
as well. 

In my testing, this fixes the performance regression.

Hannes

Re: RFR [11]: 8194985: JavaAdapterBytecodeGenerator passes invalid type descriptor to ASM

[Hannes Wallnöfer]

Actually the old implementation was a bit careless, checking just the first 
character in a switch statement for primitive and reference types, and then 
creating a *method* type in the default case, which just happened to work by 
chance for our checkcast operation. The way it is now is definitely an 
improvement.

Thanks for the reviews!

Hannes 

> Am 12.01.2018 um 13:00 schrieb Attila Szegedi <szege...@gmail.com>:
> 
> +1
> 
> That’s a weird thing to make stricter. I guess in anticipation of value types 
> which will use something other than L it can’t just substitute L anymore for 
> anything that’s not a known primitive type name.
> 
> Attila.
> 
>> On 2018. Jan 12., at 11:08, Sundararajan Athijegannathan 
>> <sundararajan.athijegannat...@oracle.com> wrote:
>> 
>> +1
>> 
>> -Sundar
>> 
>> On 12/01/18, 2:56 PM, Hannes Wallnöfer wrote:
>>> Please review:
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8194985
>>> Webrev: http://cr.openjdk.java.net/~hannesw/8194985/webrev.00/
>>> 
>>> Thanks,
>>> Hannes
> 

RFR [11]: 8194985: JavaAdapterBytecodeGenerator passes invalid type descriptor to ASM

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8194985
Webrev: http://cr.openjdk.java.net/~hannesw/8194985/webrev.00/

Thanks,
Hannes

Re: RFR:8157251:BeanLinker relinks array length operations for array types

[Hannes Wallnöfer]

Priya, 

The problem with your solution that using an object array invocation with a 
primitive array will throw a ClassCastException as it passes the isArray guard.

On the other hand, the problem with instanceof validation in Attila’s snippet 
is that it uses the concrete callsite type (e.g. String[]) instead of Object[]. 
What we’d need is a guard with an instanceof Object[] check. 

However, I don’t think a callsite for array length will usually see so many 
different array classes to go megamorphic, so I think both your solutions 
(Priya’s webrev and Attila’s code snippet) are acceptable.

Hannes

> Am 10.01.2018 um 06:42 schrieb Priya Lakshmi Muthuswamy 
> :
> 
> Hi Attila,
> 
> Thanks for the review. I just felt in the case of primitive types there will 
> be relinking.
> I tried with the below fix.
> In the case object types, I see relinking when we use Validation.INSTANCE_OF. 
> But works fine without relinking when we use Validation.IS_ARRAY
> 
> if(clazz.isArray()) {
>// Some languages won't have a notion of manipulating collections. 
> Exposing "length" on arrays as an // explicit property is beneficial for 
> them. if (clazz.getComponentType().isPrimitive()) {
>setPropertyGetter("length", MethodHandles.arrayLength(clazz), 
> ValidationType.EXACT_CLASS);
>}else {
>setPropertyGetter("length", MethodHandles.arrayLength(Object[].class), 
> ValidationType.IS_ARRAY);
>}
> 
> Thanks,
> Priya
> 
> 
> On 1/9/2018 7:51 PM, Attila Szegedi wrote:
>> This effectively reverts the combined 
>>  and 
>> .
>> 
>> That was not the intent of 8157251, though.
>> 
>> The intent of 8157251 was to use MethodHandles.arrayLength(Object[].class) 
>> for all arrays with components of reference type (and use 
>> ValidationType.INSTANCE_OF for it, as all such arrays will be instance-of 
>> Object[] through instance-of semantics for array types) and leave the 
>> linking of arrays with primitive component types as they are today.
>> 
>> So, basically, something like:
>> 
>> if(clazz.isArray()) {
>> // Some languages won't have a notion of manipulating collections. 
>> Exposing "length" on arrays as an
>> // explicit property is beneficial for them.
>> if (clazz.getComponentType().isPrimitive()) {
>> setPropertyGetter("length", MethodHandles.arrayLength(clazz), 
>> ValidationType.EXACT_CLASS);
>> } else {
>> setPropertyGetter("length", 
>> MethodHandles.arrayLength(Object[].class), ValidationType.INSTANCE_OF);
>> }
>> 
>> Obviously, this is again a tradeoff between linkage stability and 
>> performance, except that with this solution we don’t sacrifice any 
>> performance and we still increase linkage stability. Going back to 
>> Array.getLength does further increase linkage stability, but *speculatively* 
>> it doesn’t intrinsify to as lean code as MethodHandles.arrayLength does (it 
>> certainly erases the type information of the argument); someone with time on 
>> their hands should probably run some PrintAssembly tests with 
>> -XX:-TieredCompilation to see whether this is true (if not, then 8157225 and 
>> 8157250 were done for nothing and this patch undoes them anyway).
>> 
>> In any case, I don’t have a too strong opinion about this; I don’t mind even 
>> if this ends up being a reversal of 8157225; it’s just weird that we have 
>> arrayLength method handle intrinsics and not use them.
>> 
>> Attila.
>> 
>> 
>>> On Jan 9, 2018, at 8:00 AM, Priya Lakshmi Muthuswamy 
>>>  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review JDK-8157251 : BeanLinker relinks array length operations for 
>>> array types
>>> 
>>> JBS : https://bugs.openjdk.java.net/browse/JDK-8157251
>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8157251/webrev.00/
>>> 
>>> Thanks,
>>> Priya
> 

Re: RFR:8157251:BeanLinker relinks array length operations for array types

[Hannes Wallnöfer]

+1

Hannes

> Am 09.01.2018 um 14:23 schrieb Jim Laskey (Oracle) :
> 
> +1
> 
> 
>> On Jan 9, 2018, at 3:00 AM, Priya Lakshmi Muthuswamy 
>>  wrote:
>> 
>> Hi,
>> 
>> Please review JDK-8157251 : BeanLinker relinks array length operations for 
>> array types
>> 
>> JBS : https://bugs.openjdk.java.net/browse/JDK-8157251
>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8157251/webrev.00/
>> 
>> Thanks,
>> Priya
> 

Re: RFR: 8193567: Conversion of comparison nodes affects local slots in optimistic continuation

[Hannes Wallnöfer]

Thanks for the review, Attila. 

I didn’t look at the contents of the local slots. I think the root of the 
problem is that the comparison is represented as RuntimeNode in some cases and 
as BinaryNode in others, and it can switch between the two between optimistic 
recompilations (see LocalVariableTypesCalculator.leaveBinaryNode on how 
comparison nodes are replaced with RuntimeNodes depending on the types of 
sub-expressions). I doubt it is possible to guarantee these representations use 
a compatible slot layout, and even if there is, I think for JDK 10 it’s best to 
take the safe path.

Hannes


> Am 25.12.2017 um 17:27 schrieb Attila Szegedi <szege...@gmail.com>:
> 
> A sad +1. I too wish this were easier to solve. Can you point me to the code 
> parts that cause differences in local slot assignment? I guess I could try to 
> figure it out myself from that terrible expression in the test, but if you 
> have a quick explanation…
> 
> Thanks,
>  Attila.
> 
>> On Dec 21, 2017, at 4:36 PM, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> 
>> wrote:
>> 
>> Please review:
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193567
>> Webrev: http://cr.openjdk.java.net/~hannesw/8193567/webrev.00/
>> 
>> I’ve tried finding a smaller fix like just tagging the child nodes of all 
>> comparisons as non-optimistic, but that didn't fix the problem as those 
>> nodes could still have optimistic children.
>> 
>> Hannes
> 

Re: Review request for JDK-8193295: Remove no longer used COMMALEFT

[Hannes Wallnöfer]

Interesting. I bet the answer is in the commits from before we went open source.

+1

I assume this is for JDK 11?

Hannes

> Am 21.12.2017 um 19:20 schrieb Attila Szegedi :
> 
> Please review JDK-8193295 "Remove no longer used COMMALEFT" at 
>  for 
> 
> 
> I love deleting code :-)
> 
> COMMALEFT was an odd duck, as there’s not really anything in the ES 
> specification that’d require it, and we really don’t even use it for 
> anything. I had a memory that we used to use it for some behavior around 
> object literals, but I can’t find any traces of that… I run:
> 
>   hg log --template "{ifcontains('COMMALEFT', diff(), '{node} {desc}\n', 
> '')}”
> 
> and trawled through the diffs for all changesets it brought up, but I haven’t 
> found a single use of COMMALEFT ever.
> 
> Thanks,
>  Attila.

RFR: 8193567: Conversion of comparison nodes affects local slots in optimistic continuation

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8193567
Webrev: http://cr.openjdk.java.net/~hannesw/8193567/webrev.00/

I’ve tried finding a smaller fix like just tagging the child nodes of all 
comparisons as non-optimistic, but that didn't fix the problem as those nodes 
could still have optimistic children.

Hannes

Re: RFR 8193779: Fix copyright header in nashorn builtin scripts

[Hannes Wallnöfer]

+1

Hannes

> Am 19.12.2017 um 11:38 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8193799
> Webrev: http://cr.openjdk.java.net/~sundar/8193799/webrev.00/
> 
> Thanks,
> -Sundar
> 

Re: Changes in Java 9

[Hannes Wallnöfer]

Hi Nils,

Thanks for the code. Unfortunately you hit a bug in optimistic types, which is 
enabled by default in JDK 9. I’ve filed an issue for it and have a fix for it.

https://bugs.openjdk.java.net/browse/JDK-8193508

The good news is that it is quite easy to work around this bug by disabling 
optimistic types. The way to do this is to use the NashornScriptEngineFactory 
class to create the script engine:

import jdk.nashorn.api.scripting.NashornScriptEngineFactory; 

...
ScriptEngine engine = new 
NashornScriptEngineFactory().getScriptEngine("-ot=false"); 

Hannes


> Am 06.12.2017 um 22:30 schrieb Nils Kilden-Pedersen <nil...@gmail.com>:
> 
> Hannes,
> 
> I've sent an email with zip file attachment. 
> 
> Just letting you know in a separate email, in case it ends up being blocked.
> 
> Nils
> 
> On Wed, Dec 6, 2017 at 2:19 PM, Hannes Wallnöfer 
> <hannes.wallnoe...@oracle.com> wrote:
> That would be https://bugs.openjdk.java.net/ but you need an OpenJDK account 
> to file a bug.
> 
> Alternatively, if you can send me something reproduce the bug (could be a 
> small snippet of code) I can file it for you.
> 
> Hannes
> 
> 
> > Am 06.12.2017 um 16:15 schrieb Nils Kilden-Pedersen <nil...@gmail.com>:
> >
> > What's the right place to file a bug?
> >
> > On Mon, Dec 4, 2017 at 9:56 AM, Nils Kilden-Pedersen <nil...@gmail.com>
> > wrote:
> >
> >> Thanks for bringing this up. It made me realize that my post was
> >> incomplete.
> >>
> >> These are the steps I take:
> >>
> >>   1. Initial step: Call engine.compile with the CS compiler source +
> >>   CoffeeScript.compile(coffeeCode, {runtime: 'none'}); which returns
> >>   CompiledScript that is retained and re-used. This works fine, also on
> >>   JDK 9.0.1.
> >>   2. For subsequent Coffeescript -> Javascript transpilation, I then
> >>   call engine.createBindings()
> >>   3. Then bindings.put("coffeeCode", csCode) to associate the CS code
> >>   with the JS variable coffeeCode mentioned in initial step
> >>   4. Call CompiledScript.eval(bindings). This is the step that fails in
> >>   9.0.1.
> >>
> >> I’m open to better ways to achieve this, if it’s inefficient or plain
> >> wrong.
> >> ​
> >>
> >> On Mon, Dec 4, 2017 at 8:47 AM, Hannes Wallnöfer <
> >> hannes.wallnoe...@oracle.com> wrote:
> >>
> >>> Hi Nils,
> >>>
> >>> Are you just evaluating the script files you linked in your first
> >>> message, or trying to do some further processing?
> >>>
> >>> Using the jjs tool from JDK 9.0.1 I see no errors running both versions
> >>> of CoffeeScript.
> >>>
> >>> Hannes
> >>>
> >>>> Am 03.12.2017 um 21:08 schrieb Nils Kilden-Pedersen <nil...@gmail.com>:
> >>>>
> >>>> Ok, the minified vs non-minified may not be identical. I cannot find the
> >>>> default parameters in the minified version, so perhaps those are
> >>> removed.
> >>>>
> >>>> What does remain however, is that I can compile minified cs2
> >>>> <http://coffeescript.org/v2/browser-compiler/coffeescript.js> on JDK
> >>> 8_144,
> >>>> but not on 9.0.1.
> >>>>
> >>>>
> >>>> On Sun, Dec 3, 2017 at 1:39 PM, Nils Kilden-Pedersen <nil...@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> Further testing with the un-minified code (cs2
> >>>>> <https://unpkg.com/coffeescript@2.0.3/lib/coffeescript/browser.js>,
> >>> cs1
> >>>>> <https://unpkg.com/coffeescript@1.12.7/lib/coffee-script/browser.js>)
> >>>>> reveals the true problems in JDK 9.0.1
> >>>>>
> >>>>> In cs2, it’s ES6 syntax (default args), which (unexpectedly for me at
> >>>>> least), works in JDK 8:
> >>>>>
> >>>>>   :46:45 Expected , but found =
> >>>>> CoffeeScript.eval = function(code, options = {}) {
> >>>>>^ in  at line number
> >>> 46 at column number 45
> >>>>>
> >>>>> And in cs1 it’s require that’s unresolved (also works in JDK 8):
> >>>>>
> >>>>> ReferenceError: "require" is not defined in  at line number 7
> >>>>>
> >>>>> ​
>

Re: Review request for JDK-8193371: Use Dynalink REMOVE operation in Nashorn

[Hannes Wallnöfer]

Nice work indeed.

+1

Hannes

> Am 14.12.2017 um 12:42 schrieb Attila Szegedi :
> 
> Please review JDK-8193371 "Use Dynalink REMOVE operation in Nashorn" at 
>  for 
> 
> 
> It looks bigger than it is. let me quickly explain this change in detail:
> 
> There are some things that could make sense even if we didn’t refit delete 
> operator on Dynalink REMOVE.
> 
> * Fist such thing is moving the evaluation logic for the delete operator from 
> AssignSymbols into CodeGenerator. That way, we eliminated a need to carry 
> information from AS to CG in a RuntimeNode, and we could retire all of 
> DELETE, SLOW_DELETE, and FAIL_DELETE RuntimeNode enums. It also allowed us to 
> have cleaner expression of the code generator, e.g strict flag is handled 
> more elegantly when deleting an IdentNode, and also we can just emit 
> invocation of ScriptObject.delete on the scope object for IdentNode removals. 
> There’s overall stronger typing (e.g. slow delete implementation has a better 
> signature).
> 
> * By not erasing “delete” UnaryNodes in AssignSymbols anymore, I had to take 
> a bit of special care in LocalVariableTypesCalculator to not treat deletes of 
> IdentNodes as uses.
> 
> * The changes in control flow of AbstractJavaLinker and BeanLinker 
> getGuardedInvocationComponent are actual bug fixes that became apparent to me 
> when I realized while writing the tests that the code as it was was failing 
> to link REMOVE:PROPERTY|ELEMENT; it was incorrectly not trying all the 
> namespaces it should’ve. (The bug was more generic than just that one 
> operation combination: it’d affect e.g. SET:METHOD|PROPERTY too and some 
> other combinations).
> 
> As for actual delete operator through Dynalink:
> 
> * MethodEmitter gained methods for emitting dynamic call sites for removal. 
> Those are very similar to existing ones for dynamic getters and setters, 
> nothing special there.
> 
> * The logic formerly in ScriptRuntime.DELETE has now been spread into 
> relevant linkers: ScriptObject.lookup, JSObjectLinker, Undefined.lookup, and 
> NashornBottomLinker. The case in ScriptRuntime.DELETE for “if 
> (JSType.isPrimitive(obj))” doesn’t even have to be handled separately, 
> primitive linking that instantiates wrappers takes care nicely of it.
> 
> * That said, ScriptRuntime.DELETE had a final “return true” fallback for 
> POJOs, and I decided to improve on it. For one thing, theres an automatic 
> improvement because delete now works as expected on Java lists and maps just 
> by virtue of Dynalink providing that functionality. Another improvement that 
> I decided to introduce manually is found in the way NashornBottomLinker 
> handles delete on POJOs, specifically the uses of isNonConfigurableProperty. 
> I tried treating all properties and methods of POJOs as if they were 
> non-configurable properties (we try to treat them like that elsewhere in the 
> code as well), so attempts to delete them will return false (non-strict) or 
> throw a TypeError (strict), in accordance with ES 8.12.7 [[Delete]]. The 
> newly added test file tries to exercise all new behavior.
> 
> * Finally, NashornCallSiteDescriptor now needs 4 bits for the operation as we 
> went from 8 operations to 10 with “delete obj.name” and “delete obj[index]". 
> This again halved the number of program points available to 131072, but 
> that's fortunately still about 10x what is really needed, so we should be 
> good.
> 
> Thanks,
>  Attila.
> 

RFR: 8193508: Expressions in split literals must never be optimistic

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8193508
Webrev: http://cr.openjdk.java.net/~hannesw/8193508/webrev.00/

The actual fix is quite simple: stop visiting when we encounter a split object 
or array literal in OptimisticTypesCalculator.

There are a few non-essential changes:
 - rename methods and fields in CodeGeneratorLexicalContext from *SplitNode to 
*SplitLiteral as the former is misleading (no actual SplitNodes around)
 - make sure we don’t have optimistic operations unless we generate optimistic 
code to avoid the situation that enabled this bug
 - add logging for split array and object nodes in Splitter
 - add some @Ignore annotations to make nodes behave in IR printer

Thanks,
Hannes

Re: RFR:8191301:JavaImporter fails to resolve imported elements within functions, that contain too many statements

[Hannes Wallnöfer]

+1, and I like the fact that the patch has become smaller.

Hannes


> Am 12.12.2017 um 13:32 schrieb Priya Lakshmi Muthuswamy 
> :
> 
> Hi,
> 
> Kindly review. I have modified the fix to work with multiple with scopes.
> 
> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.01/
> 
> Thanks,
> Priya
> On 12/5/2017 12:54 PM, Priya Lakshmi Muthuswamy wrote:
>> Hi Attila,
>> 
>> Thanks for review.
>> Yes when I checked with two with scopes as suggested(JavaImporter as outer), 
>> current fix doesn't work. I will work on that.
>> 
>> Thanks,
>> Priya
>> On 12/5/2017 12:12 PM, Attila Szegedi wrote:
>>> Hm… this seems to be an issue with shared scope calls; that’s why it’s 
>>> sensitive to the number of similar statements.
>>> 
>>> That said, here’s some thoughts:
>>> 
>>> 1. Instead of
>>> 
>>> if (self instanceof ScriptObject && 
>>> ((ScriptObject)self).hasWithScope()) {
>>> 
>>> you should be able to just use
>>> 
>>> if (self instanceof ScriptObject) {
>>> 
>>> As then you’ll evaluate getWithScopeObject and test it for being null 
>>> anyway. This way, you avoid walking the prototype chain twice.
>>> 
>>> 2. That said, I guess hasWithScope could be reimplemented simply as
>>> 
>>> public boolean hasWithScope() {
>>> return getWithScopeObject() != null;
>>> }
>>> 
>>> as both have very similar code, so it’d reduce to it nicely. (I understand 
>>> that you haven’t changed that, but since you were in the vicinity of that 
>>> code, you might as wel do it… It’s also fine if you leave it alone as it 
>>> is.)
>>> 
>>> 3. One of the statements in the test is indented differently than the 
>>> others.
>>> 
>>> 4. What happens if there’s _two_ with scopes, and the JavaImporter is in 
>>> the outer one? Does this fix still work? E.g.:
>>> 
>>> var imports = new JavaImporter(java.lang);
>>> var dummy = { x: 42, y: 13 }
>>> with (imports) {
>>>  with (dummy) {
>>>  function func() {
>>>  System.out.println('a');
>>> System.out.println('a');
>>> System.out.println('a');
>>>  System.out.println('a');
>>> System.out.println('a');
>>> System.out.println('a');
>>> System.out.println('a');
>>>  };
>>>  func();
>>> }
>>> }
>>> 
>>> Attila.
>>> 
 On Dec 5, 2017, at 7:13 AM, Priya Lakshmi Muthuswamy 
  wrote:
 
 Hi,
 
 please review JDK-8191301 : JavaImporter fails to resolve imported 
 elements within functions, that contain too many statements
 
 JBS : https://bugs.openjdk.java.net/browse/JDK-8191301
 webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.00/
 
 Thanks,
 Priya
 
 
>> 
> 

Re: Any news from JDK-8151981 in Java8 ?

[Hannes Wallnöfer]

Hi Rémi,

Am 12.12.2017 um 14:52 schrieb Remi Forax <fo...@univ-mlv.fr>:
> 
> Hi Hannes,
> it can also be Nashorn triggering too much deoptimizations creating a 
> deoptimization storm.

I agree. But given that the problem manifests itself in java.lang.invoke I 
think the first step is to understand what is going wrong there. Without a 
better understanding of the problem it is very hard to provide provide any help.

Hannes 


> Rémi
> 
> - Mail original -----
>> De: "Hannes Wallnöfer" <hannes.wallnoe...@oracle.com>
>> À: "João Paulo Varandas" <joaovaran...@inpaas.com>
>> Cc: nashorn-dev@openjdk.java.net
>> Envoyé: Mardi 12 Décembre 2017 14:11:49
>> Objet: Re: Any news from JDK-8151981 in Java8 ?
> 
>> Hi João,
>> 
>> this functionality is part of java.lang.invoke package, not Nashorn, so we 
>> can’t
>> help you here.
>> 
>> The best place to ask about this would be the core-libs-dev mailing list.
>> 
>> Regards,
>> Hannes
>> 
>> 
>>> Am 11.12.2017 um 23:07 schrieb João Paulo Varandas 
>>> <joaovaran...@inpaas.com>:
>>> 
>>> Hi guys!
>>> 
>>> We are also experiencing some odd issues here with setCallSiteTargetNormal.
>>> https://bugs.openjdk.java.net/browse/JDK-8151981
>>> 
>>> Our cenario is:
>>> 
>>> - Web Application using Tomcat;
>>> - 8 HTTP Threads;
>>> - A single ScriptEngine for the whole application;
>>> 
>>> When a request hits the server, that request may be processed by a Nashorn
>>> Script, this script is evaluated during runtime, run queries (Jdbc), and
>>> return results (Maps and/or Collections) that are serialized to JSON before
>>> being written to the response.
>>> 
>>> The problem is... sometimes 7 or more threads are getting stuck in the
>>> setCallSiteTargetNormal and I have no clue why is that happening. Could you
>>> guys help me out troubleshooting this? - Or, if there's a bug, what are the
>>> recomended fixes I should do?
>>> 
>>> 
>>> By the way, those are not freshly created ScriptEngines (which differ from
>>> the normal issues related in the internet).
>>> 
>>> --
>>> "Esta mensagem, incluindo seus anexos, pode conter informacoes
>>> confidenciais e privilegiadas.
>>> Se voce a recebeu por engano, solicitamos que a apague e avise o remetente
>>> imediatamente.
>>> Opinioes ou informacoes aqui contidas nao refletem necessariamente a
>>> posicao oficial da Plusoft."
>>> 
>>> "Antes de imprimir, pense em sua responsabilidade e compromisso com o MEIO
>>> AMBIENTE"

Re: RFR: 8069338: Implement sharedScopeCall for optimistic types

[Hannes Wallnöfer]

Thanks for the review, Sundar.

I think making thresholds configurable is a good idea. I’ve uploaded a new 
webrev where SHARED_CALL_THRESHOLD and SHARED_GET_THRESHOLD are configurable 
via „nashorn.shared.scope.call.threshold“ and 
„nashorn.shared.scope.get.threshold“ system properties, respectively. 

http://cr.openjdk.java.net/~hannesw/8069338/webrev.02/

Hannes

> Am 12.12.2017 um 14:00 schrieb Sundararajan Athijegannathan 
> <sundararajan.athijegannat...@oracle.com>:
> 
> +1
> 
> PS. Not sure if it is worth making SHARED_GET_THRESHOLD configurable via 
> System property?
> 
> -Sundar
> 
> On 12/12/17, 5:51 PM, Hannes Wallnöfer wrote:
>> Thanks for the review, Attila.
>> 
>> I’ve uploaded a new webrev with your suggested changes (including making 
>> ScriptObject.getProto(int) accept 0 argument and refactoring GET_PROTO code 
>> in CodeGenerator into a method).
>> 
>> I have also done extensive performance testing over the last days and found 
>> that for some reason, shared methods for variable access has a very 
>> detrimental effect on performance for some reason. My theory is that we 
>> don’t save as much by making property access shared, and the flood of 
>> additional methods has some significant cost on codegen, runtime, JIT etc. 
>> Based on this I decided to increase the threshold for shared property access 
>> again to 100, which performs noticeably better than with the lower threshold 
>> in some benchmarks.
>> 
>> New webrev is here:
>> 
>> http://cr.openjdk.java.net/~hannesw/8069338/webrev.01/
>> 
>> Hannes
>> 
>> 
>>> Am 11.12.2017 um 11:55 schrieb Attila Szegedi<szege...@gmail.com>:
>>> 
>>> Reviewing this now, thanks for going ahead and implementing it!
>>> 
>>> So what you are saying about slow scopes is that shared scopes were 
>>> disabled anyway with them for some time, so you didn’t really take away 
>>> functionality, just cleaned up the remains?
>>> 
>>> So, some remarks:
>>> 
>>> 1. I see the bytecode shape you wrote around invocation of 
>>> ScriptObject.getProto(int), and I realized it’s weird it asserts n>  0 and 
>>> it has the first getProto() unrolled before the loop. I’m pretty sure it 
>>> can just look like this:
>>> 
>>> public final ScriptObject getProto(final int n) {
>>> ScriptObject p = this;
>>> for (int i = n; i-->  0;) {
>>> p = p.getProto();
>>> }
>>> return p;
>>> }
>>> 
>>> That assert was there because we knew that it’s only CodeGenerator that 
>>> emits calls to it, and it will always emit invocations with n>  0, but now 
>>> we’d have a justification for invoking it with n == 0, so I think it’s fine 
>>> to do it and avoid the no_proto_call branch in shared scope call bytecode. 
>>> It’s not a big deal even if you leave it as-is, though.
>>> 
>>> To be entirely pedantic, that assert in getProto should’ve read “n>  1” 
>>> before because the CodeGenerator was guaranteed to emit invocations to it 
>>> with n>  1, so a really pedantic implementation of it should’ve read:
>>> 
>>> public final ScriptObject getProto(final int n) {
>>> assert n>  1;
>>> ScriptObject p = getProto().getProto();
>>> for (int i = n - 1; --i>  0;) {
>>> p = p.getProto();
>>> }
>>> return p;
>>> }
>>> 
>>> (God, I really hope everyone reading this realizes I’m joking with this 
>>> last one.)
>>> 
>>> FWIW, we could also factor out the:
>>> 
>>> if (depth>  1) {
>>> method.load(depth);
>>> method.invoke(ScriptObject.GET_PROTO_DEPTH);
>>> } else {
>>> method.invoke(ScriptObject.GET_PROTO);
>>> }
>>> 
>>> pattern in CodeGenerator into its own little “emitGetProto(int depth)” 
>>> method, as it appears twice (and make sure it asserts on depth>  0 ;-) )
>>> 
>>> 2. For that invocation of UnwarrantedOptimismException.withProgramPoint - 
>>> I’d create a Call objects using CompilerConstants.virtualCallNoLookup and 
>>> stash it in a static final field either in SharedScopeCall or 
>>> UnwarrantedOptimismException; it’s more typesafe than writing type 
>>> descriptors as strings.
>>> 
>>> 3. So, UOE.hasPrimitiveReturnValue was unused, I take it?
>>> 
>>> 3. In getStaticSignature you have another occurrence of number 3 that you 
>>

Re: Any news from JDK-8151981 in Java8 ?

[Hannes Wallnöfer]

Hi João,

this functionality is part of java.lang.invoke package, not Nashorn, so we 
can’t help you here. 

The best place to ask about this would be the core-libs-dev mailing list.

Regards,
Hannes


> Am 11.12.2017 um 23:07 schrieb João Paulo Varandas :
> 
> Hi guys!
> 
> We are also experiencing some odd issues here with setCallSiteTargetNormal.
> https://bugs.openjdk.java.net/browse/JDK-8151981
> 
> Our cenario is:
> 
> - Web Application using Tomcat;
> - 8 HTTP Threads;
> - A single ScriptEngine for the whole application;
> 
> When a request hits the server, that request may be processed by a Nashorn
> Script, this script is evaluated during runtime, run queries (Jdbc), and
> return results (Maps and/or Collections) that are serialized to JSON before
> being written to the response.
> 
> The problem is... sometimes 7 or more threads are getting stuck in the
> setCallSiteTargetNormal and I have no clue why is that happening. Could you
> guys help me out troubleshooting this? - Or, if there's a bug, what are the
> recomended fixes I should do?
> 
> 
> By the way, those are not freshly created ScriptEngines (which differ from
> the normal issues related in the internet).
> 
> -- 
> "Esta mensagem, incluindo seus anexos, pode conter informacoes 
> confidenciais e privilegiadas. 
> Se voce a recebeu por engano, solicitamos que a apague e avise o remetente 
> imediatamente. 
> Opinioes ou informacoes aqui contidas nao refletem necessariamente a 
> posicao oficial da Plusoft."
> 
> "Antes de imprimir, pense em sua responsabilidade e compromisso com o MEIO 
> AMBIENTE"
> 

Re: Review request for JDK-8193298: Don't run javadoc with test.single

[Hannes Wallnöfer]

+1

Hannes

> Am 10.12.2017 um 17:26 schrieb Attila Szegedi :
> 
> Please review JDK-8193298 "Don't run javadoc with test.single" at 
>  for 
> 
> 
> Thanks,
>  Attila.

Re: Review request for JDK-8191905: Add a REMOVE value to jdk.dynalink.StandardOperation

[Hannes Wallnöfer]

+1

Hannes

> Am 08.12.2017 um 12:13 schrieb Attila Szegedi :
> 
> Please review JDK-8191905 "Add a REMOVE value to 
> jdk.dynalink.StandardOperation" at 
>  for 
> 
> 
> The public API changes have an approved CSR. Note that this adds a REMOVE 
> operation to Dynalink and a default implementation for Lists and Maps to 
> BeanLinker, but stops short of retrofitting Nashorn’s delete operator - 
> that’s the next step I’m yet to implement.
> 
> Thanks,
>  Attila.

Re: RFR: 8069338: Implement sharedScopeCall for optimistic types

[Hannes Wallnöfer]

Thanks for the review, Attila.

I’ve uploaded a new webrev with your suggested changes (including making 
ScriptObject.getProto(int) accept 0 argument and refactoring GET_PROTO code in 
CodeGenerator into a method). 

I have also done extensive performance testing over the last days and found 
that for some reason, shared methods for variable access has a very detrimental 
effect on performance for some reason. My theory is that we don’t save as much 
by making property access shared, and the flood of additional methods has some 
significant cost on codegen, runtime, JIT etc. Based on this I decided to 
increase the threshold for shared property access again to 100, which performs 
noticeably better than with the lower threshold in some benchmarks.

New webrev is here:

http://cr.openjdk.java.net/~hannesw/8069338/webrev.01/

Hannes


> Am 11.12.2017 um 11:55 schrieb Attila Szegedi <szege...@gmail.com>:
> 
> Reviewing this now, thanks for going ahead and implementing it!
> 
> So what you are saying about slow scopes is that shared scopes were disabled 
> anyway with them for some time, so you didn’t really take away functionality, 
> just cleaned up the remains?
> 
> So, some remarks:
> 
> 1. I see the bytecode shape you wrote around invocation of 
> ScriptObject.getProto(int), and I realized it’s weird it asserts n > 0 and it 
> has the first getProto() unrolled before the loop. I’m pretty sure it can 
> just look like this:
> 
> public final ScriptObject getProto(final int n) {
> ScriptObject p = this;
> for (int i = n; i-- > 0;) {
> p = p.getProto();
> }
> return p;
> }
> 
> That assert was there because we knew that it’s only CodeGenerator that emits 
> calls to it, and it will always emit invocations with n > 0, but now we’d 
> have a justification for invoking it with n == 0, so I think it’s fine to do 
> it and avoid the no_proto_call branch in shared scope call bytecode. It’s not 
> a big deal even if you leave it as-is, though.
> 
> To be entirely pedantic, that assert in getProto should’ve read “n > 1” 
> before because the CodeGenerator was guaranteed to emit invocations to it 
> with n > 1, so a really pedantic implementation of it should’ve read:
> 
> public final ScriptObject getProto(final int n) {
> assert n > 1;
> ScriptObject p = getProto().getProto();
> for (int i = n - 1; --i > 0;) {
> p = p.getProto();
> }
> return p;
> }
> 
> (God, I really hope everyone reading this realizes I’m joking with this last 
> one.)
> 
> FWIW, we could also factor out the:
> 
> if (depth > 1) {
> method.load(depth);
> method.invoke(ScriptObject.GET_PROTO_DEPTH);
> } else {
> method.invoke(ScriptObject.GET_PROTO);
> }
> 
> pattern in CodeGenerator into its own little “emitGetProto(int depth)” 
> method, as it appears twice (and make sure it asserts on depth > 0 ;-) )
> 
> 2. For that invocation of UnwarrantedOptimismException.withProgramPoint - I’d 
> create a Call objects using CompilerConstants.virtualCallNoLookup and stash 
> it in a static final field either in SharedScopeCall or 
> UnwarrantedOptimismException; it’s more typesafe than writing type 
> descriptors as strings.
> 
> 3. So, UOE.hasPrimitiveReturnValue was unused, I take it?
> 
> 3. In getStaticSignature you have another occurrence of number 3 that you can 
> replace with FIXED_PARAM_COUNT :-)
> 
> But these are all minor points… overall, this looks really good.
> 
> Attila.
> 
>> On Dec 6, 2017, at 6:43 PM, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> 
>> wrote:
>> 
>> Please review: 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8069338
>> Webrev: http://cr.openjdk.java.net/~hannesw/8069338/webrev.00/
>> 
>> This implements shared scope calls for optimistic vars/calls. I followed 
>> Attila’s blueprint in the bug description to the last detail - thanks for 
>> figuring it all out.
>> 
>> Some adjustments and cleanups I did along the way:
>> 
>> - Originally shared scope calls were designed to support slow scope vars 
>> (with/eval). However, that feature was disabled later on. I refactored the 
>> code to make it clear that we only do shared scope calls for fast scope 
>> operations.
>> - I unified the threshold for all shared scope operations to 5. I don’t 
>> think there is a reason for different thresholds for different operations. I 
>> did some testing, using 5 as threshold appeared to be a sensible value.
>> 
>> Running all of the  octane benchmark I see a decrease in created callsites 
>> (200 000 to 178 000) and a slight decrease in callsite invalidations.
>> 
>> Hannes
> 

Re: RFR:JDK-8193137:Nashorn crashes when given an empty script file

[Hannes Wallnöfer]

+1

Hannes

> Am 08.12.2017 um 10:26 schrieb Priya Lakshmi Muthuswamy 
> :
> 
> Thanks Sundar. I have modified the test.
> 
> updated webrev : http://cr.openjdk.java.net/~pmuthuswamy/8193137/webrev.01/
> 
> Thanks,
> Priya
> On 12/8/2017 2:40 PM, Sundararajan Athijegannathan wrote:
>> Looks good. The test uses $EXEC("rm..."). Better to avoid that [what about 
>> Windows?] - use Java API to delete file instead./
>> /
>> -Sundar
>> 
>> On 08/12/17, 2:09 PM, Priya Lakshmi Muthuswamy wrote:
>>> Hi,
>>> 
>>> Please review JDK-8193137 : Nashorn crashes when given an empty script file
>>> 
>>> JBS : https://bugs.openjdk.java.net/browse/JDK-8193137
>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8193137/webrev.00/
>>> 
>>> Thanks,
>>> Priya
> 

Re: RFR: 8134516(Move getInvokeByName and getDynamicInvoker methods from Global to Context)

[Hannes Wallnöfer]

I have some doubts about whether this is worth it. The idea was to be able to 
use these internal callsites between various globals without relinking. But 
since built-in prototypes in different globals have different property maps 
that doesn’t really happen. That matches your observation that fewer callsites 
are created with your patch, but more callsites are invalidated. 

I think if we want to do this, we also have to make sure builtins in different 
globals use the same property map. Otherwise it’s probably better not to make 
this change.

Hannes


> Am 06.12.2017 um 16:47 schrieb Srinivas Dama :
> 
> Hi,
> 
> Please review http://cr.openjdk.java.net/~sdama/8134516/webrev.01/ for 
> https://bugs.openjdk.java.net/browse/JDK-8134516 
> 
> Regards,
> Srinivas

RFR: 8069338: Implement sharedScopeCall for optimistic types

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8069338
Webrev: http://cr.openjdk.java.net/~hannesw/8069338/webrev.00/

This implements shared scope calls for optimistic vars/calls. I followed 
Attila’s blueprint in the bug description to the last detail - thanks for 
figuring it all out.

Some adjustments and cleanups I did along the way:

 - Originally shared scope calls were designed to support slow scope vars 
(with/eval). However, that feature was disabled later on. I refactored the code 
to make it clear that we only do shared scope calls for fast scope operations.
 - I unified the threshold for all shared scope operations to 5. I don’t think 
there is a reason for different thresholds for different operations. I did some 
testing, using 5 as threshold appeared to be a sensible value.

Running all of the  octane benchmark I see a decrease in created callsites (200 
000 to 178 000) and a slight decrease in callsite invalidations.

Hannes

Re: Review request for JDK-8192970: Element getters/setters with fixed key fail to link properly

[Hannes Wallnöfer]

D'oh! I blindly clicked on that links of course :)

> Am 04.12.2017 um 17:05 schrieb Attila Szegedi <szege...@gmail.com>:
> 
> I know, it’s subtle; that’s why I decided to describe it in detail :-)
> 
>> On 2017. Dec 4., at 16:36, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> 
>> wrote:
>> 
>> Took me some time to spot the difference in behaviour (adaptation of method 
>> type in Binder)!
>> 
>> +1
>> 
>> Hannes
>> 
>>> Am 04.12.2017 um 13:40 schrieb Attila Szegedi <szege...@gmail.com>:
>>> 
>>> Please review JDK-8192970 "Element getters/setters with fixed key fail to 
>>> link properly" at <http://cr.openjdk.java.net/~attila/8192970/webrev.jdk> 
>>> for <https://bugs.openjdk.java.net/browse/JDK-8192970>
>>> 
>>> Note that this is unrelated to the previous refactoring (8191878), it is 
>>> present even before that and also in JDK9, I’ll have to separately produce 
>>> a backport fix for it. I discovered this while trying to expand the test 
>>> coverage (and good thing that I did, too…)
>>> 
>>> The gist of the problem is that convertArgToNumber as it was before will 
>>> fail with IndexOutOfBoundsException invoking parameterType(1) on method 
>>> type for fixed-key getters, as they only have one parameter at index 0. 
>>> 
>>> The fix is to move the logic into the Binder and use the methodType it 
>>> manages, not the call site method type. This works because Binder always 
>>> works with a method type as if the getter/setter were variable-key (it adds 
>>> back the parameter type in its constructor if the operation is for a fixed 
>>> key).
>>> 
>>> Thanks,
>>> Attila.
>> 
> 

Re: Review request for JDK-8192970: Element getters/setters with fixed key fail to link properly

[Hannes Wallnöfer]

Took me some time to spot the difference in behaviour (adaptation of method 
type in Binder)!

+1

Hannes

> Am 04.12.2017 um 13:40 schrieb Attila Szegedi :
> 
> Please review JDK-8192970 "Element getters/setters with fixed key fail to 
> link properly" at  for 
> 
> 
> Note that this is unrelated to the previous refactoring (8191878), it is 
> present even before that and also in JDK9, I’ll have to separately produce a 
> backport fix for it. I discovered this while trying to expand the test 
> coverage (and good thing that I did, too…)
> 
> The gist of the problem is that convertArgToNumber as it was before will fail 
> with IndexOutOfBoundsException invoking parameterType(1) on method type for 
> fixed-key getters, as they only have one parameter at index 0. 
> 
> The fix is to move the logic into the Binder and use the methodType it 
> manages, not the call site method type. This works because Binder always 
> works with a method type as if the getter/setter were variable-key (it adds 
> back the parameter type in its constructor if the operation is for a fixed 
> key).
> 
> Thanks,
>  Attila.

Re: Changes in Java 9

[Hannes Wallnöfer]

Hi Nils,

Are you just evaluating the script files you linked in your first message, or 
trying to do some further processing?

Using the jjs tool from JDK 9.0.1 I see no errors running both versions of 
CoffeeScript.

Hannes

> Am 03.12.2017 um 21:08 schrieb Nils Kilden-Pedersen :
> 
> Ok, the minified vs non-minified may not be identical. I cannot find the
> default parameters in the minified version, so perhaps those are removed.
> 
> What does remain however, is that I can compile minified cs2
>  on JDK 8_144,
> but not on 9.0.1.
> 
> 
> On Sun, Dec 3, 2017 at 1:39 PM, Nils Kilden-Pedersen 
> wrote:
> 
>> Further testing with the un-minified code (cs2
>> , cs1
>> )
>> reveals the true problems in JDK 9.0.1
>> 
>> In cs2, it’s ES6 syntax (default args), which (unexpectedly for me at
>> least), works in JDK 8:
>> 
>>:46:45 Expected , but found =
>>  CoffeeScript.eval = function(code, options = {}) {
>> ^ in  at line number 46 at 
>> column number 45
>> 
>> And in cs1 it’s require that’s unresolved (also works in JDK 8):
>> 
>> ReferenceError: "require" is not defined in  at line number 7
>> 
>> ​
>> 
>> On Sun, Dec 3, 2017 at 11:02 AM, Nils Kilden-Pedersen 
>> wrote:
>> 
>>> I just switched to the Java 9.0.1 runtime, and Nashorn now fails to parse
>>> the Coffeescript compiler, which worked fine in Java 8_144.
>>> 
>>> Coffeescript 2 compiler: http://coffeescript.org/v2/bro
>>> wser-compiler/coffeescript.js
>>> 
>>> v1 also fails: http://coffeescript.org/v1/bro
>>> wser-compiler/coffee-script.js
>>> 
>>> Exception is:
>>> 
>>> javax.script.ScriptException: SyntaxError: unexpected  in  at line 
>>> number 39 at column number 249736
>>>  at 
>>> jdk.scripting.nashorn/jdk.nashorn.api.scripting.NashornScriptEngine.throwAsScriptException(NashornScriptEngine.java:469)
>>>  at 
>>> jdk.scripting.nashorn/jdk.nashorn.api.scripting.NashornScriptEngine.evalImpl(NashornScriptEngine.java:425)
>>>  at 
>>> jdk.scripting.nashorn/jdk.nashorn.api.scripting.NashornScriptEngine.access$300(NashornScriptEngine.java:72)
>>>  at 
>>> jdk.scripting.nashorn/jdk.nashorn.api.scripting.NashornScriptEngine$3.eval(NashornScriptEngine.java:513)
>>>  at java.scripting/javax.script.CompiledScript.eval(CompiledScript.java:92)
>>>  at scuff.js.CoffeeScriptCompiler.compile(CoffeeScriptCompiler.scala:115)
>>>  at 
>>> scuff.js.TestCoffeeScriptCompiler.cs2(TestCoffeeScriptCompiler.scala:114)
>>>  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
>>> Method)
>>>  at 
>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>>  at 
>>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>>  at java.base/java.lang.reflect.Method.invoke(Method.java:564)
>>>  at 
>>> org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
>>>  at 
>>> org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
>>>  at 
>>> org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
>>>  at 
>>> org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
>>>  at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
>>>  at 
>>> org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
>>>  at 
>>> org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
>>>  at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
>>>  at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
>>>  at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
>>>  at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
>>>  at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
>>>  at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
>>>  at 
>>> org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
>>>  at 
>>> org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
>>>  at 
>>> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:539)
>>>  at 
>>> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:761)
>>>  at 
>>> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:461)
>>>  at 
>>> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:207)
>>> Caused by: :39:249736 SyntaxError: unexpected
>>>  at 
>>> jdk.scripting.nashorn/jdk.nashorn.internal.objects.NativeError.initException(NativeError.java:135)
>>>  at 
>>> 

Re: RFR: 8191891: Update minumum Ant version in Nashorn build.xml

[Hannes Wallnöfer]

Thanks.

> Am 28.11.2017 um 15:59 schrieb Sundararajan Athijegannathan 
> <sundararajan.athijegannat...@oracle.com>:
> 
> PS. Is there a doc where we documented this?

I don’t think there is. I have grepped for ‚Ant‘ and the version number in 
docs/nashorn and didn’t find anything.

Hannes

> 
> -Sundar
> 
> On 28/11/17, 7:56 PM, Hannes Wallnöfer wrote:
>> Please review:
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191891
>> Webrev: http://cr.openjdk.java.net/~hannesw/8191891/webrev.00/
>> 
>> Thanks,
>> Hannes

RFR: 8191891: Update minumum Ant version in Nashorn build.xml

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8191891
Webrev: http://cr.openjdk.java.net/~hannesw/8191891/webrev.00/

Thanks,
Hannes

RFR: 8059835: Optimistic splitting doesn't work with let and const

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8059835
Webrev: http://cr.openjdk.java.net/~hannesw/8059835/webrev.00/

Thanks,
Hannes

Re: Review request for JDK-8191878: Reduce code duplication in BeanLinker

[Hannes Wallnöfer]

Hi Attila,

> Am 26.11.2017 um 19:51 schrieb Attila Szegedi :
> 
> I’m able to spend some free time again on OpenJDK development, and one thing 
> I have on my plate is adding a REMOVE operation to Dynalink.
> 

great!

> This CR however, is not yet it: that will require a CCC sponsorship etc. as 
> I’ll be adding a new enum value to StandardOperations, and I will follow up 
> separately once I’m done with it. 

I was going to point you to the new CSR issue type, but I see you found your 
way already.

> What this is a refactoring of BeanLinker code to eliminate most of the code 
> duplication between getElementGetter and getElementSetter methods. This would 
> be justified on its own, but once I need to add the getElementRemover method, 
> it will really pay off as then we won’t have triplication of similar code.
> 
> With that in mind, please review JDK-8191878 "Reduce code duplication in 
> BeanLinker" at  for 
> 
> 

+1

Hannes

> Thanks,
>  Attila.
> 

Re: RFR 8135178: importPackage not working even with load "Mozilla compatibility script"

[Hannes Wallnöfer]

+1

Hannes


> Am 27.11.2017 um 11:24 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8135178
> Webrev:  http://cr.openjdk.java.net/~sundar/8135178/webrev.00/
> 
> Thanks,
> -Sundar

Re: RFR: 8191819: String.prototype.match should follow ES6 specification

[Hannes Wallnöfer]

Sorry, the test in the first webrev uses replace() instead of match(), which 
already works as expected.

New webrev with correct test is here:  
http://cr.openjdk.java.net/~hannesw/8191819/webrev.01/

Hannes

 
> Am 23.11.2017 um 14:18 schrieb Hannes Wallnöfer 
> <hannes.wallnoe...@oracle.com>:
> 
> Please review: 
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8191819
> Webrev: http://cr.openjdk.java.net/~hannesw/8191819/webrev/
> 
> Thanks,
> Hannes

RFR: 8191819: String.prototype.match should follow ES6 specification

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8191819
Webrev: http://cr.openjdk.java.net/~hannesw/8191819/webrev/

Thanks,
Hannes

Re: RFR 8191810: jjs should avoid hard coded javadoc base url for shift-tab documentation feature

[Hannes Wallnöfer]

+1

Hannes

> Am 23.11.2017 um 09:52 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8191810
> Webrev: http://cr.openjdk.java.net/~sundar/8191810/webrev.00/
> 
> Thanks,
> -Sundar

Re: RFR 8191771: nashorn ant makefile uses javadoc -link which may fail

[Hannes Wallnöfer]

+1

Hannes

> Am 22.11.2017 um 17:43 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8191771
> Webrev: http://cr.openjdk.java.net/~sundar/8191771/webrev.00/
> 
> Thanks,
> -Sundar

Re: RFR 8191468: jdk.scripting.nashorn.shell (jjs) module should use optional dependency for java.compiler module

[Hannes Wallnöfer]

Looks good!

Hannes

> Am 17.11.2017 um 07:30 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug:  https://bugs.openjdk.java.net/browse/JDK-8191468
> Webrev: http://cr.openjdk.java.net/~sundar/8191468/webrev.00/index.html
> 
> Thanks,
> -Sundar

Re: RFR 8068741: javax.script.ScriptEngineFactory.getMethodCallSyntax() spec allows null passed as an object

[Hannes Wallnöfer]

+1

Hannes

> Am 15.11.2017 um 18:05 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8068741
> Webrev:  http://cr.openjdk.java.net/~sundar/8068741/webrev.00/index.html
> 
> Thanks,
> -Sundar

RFR: 8191133: Ant task to fetch underscore.js requires gzip decoding option

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8191133
Webrev: http://cr.openjdk.java.net/~hannesw/8191133/webrev/

Thanks,
Hannes

RFR: 8185119: Uninitialized const when using multiple threads

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8185119
Webrev: http://cr.openjdk.java.net/~hannesw/8185119/webrev/

Thanks,
Hannes

RFR: 8191131: Nashorn test comparator breaks comparator contract

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8191131
Webrev: http://cr.openjdk.java.net/~hannesw/8191131/webrev/

Thanks,
Hannes

RFR: 8190427 : Test for JDK-8165198 fails intermittently because of GC

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8190427
Webrev: http://cr.openjdk.java.net/~hannesw/8190427/webrev.02/

This started as simple bug fix but turned into a full refactoring of the code 
that manages property switch points. However, I do think it is actually the 
only reasonable way too fix this bug, with the side benefit that the code 
becomes simpler.

The gist of this change is that PropertyMap no longer acts as property change 
listener of parent maps. Instead, the switch points are managed and invalidated 
directly in the PropertySwitchPoints class (renamed from PropertyListeners). 
Thus, we no longer depend on PropertyMaps being kept around, which was the root 
of the bug, and there’s one less level of indirection. 

I also replaced the property change callback methods in PropertyMap with a 
single propertyChanged method because it’s now the same code for all types of 
property change.

I spent quite some time testing behaviour of the new code, and it’s actually as 
good or slightly better than the old one in terms of switch points created and 
invalidated. I’m a bit embarrassed I made this more complex than it had to be 
the first time around.

Hannes

Re: RFR 8190795: jjs should show javadoc for java methods on shift-tab

[Hannes Wallnöfer]

Looks good!

Hannes


> Am 06.11.2017 um 15:37 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8190795
> Webrev: http://cr.openjdk.java.net/~sundar/8190795/webrev.00/
> 
> Piggybacking refactoring of other functions implemented in script (browse, 
> load etc.) so that we can avoid multi-line hard coded string in java sources.
> 
> -Sundar

Re: RFR 8190698: jjs tool of jdk.scripting.nashorn.shell module should not statically depend on java.desktop

[Hannes Wallnöfer]

+1

Hannes

> Am 03.11.2017 um 10:10 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8190698
> Webrev: http://cr.openjdk.java.net/~sundar/8190698/webrev.00
> 
> Thanks,
> -Sundar

Re: RFR:JDK-8186807:JSObject gets ScriptFunction when ScriptObjectMirror is expected

[Hannes Wallnöfer]

+1

Hannes

> Am 03.11.2017 um 13:27 schrieb Sundararajan Athijegannathan 
> :
> 
> +1
> 
> -Sundar
> 
> On 03/11/17, 5:15 PM, Priya Lakshmi Muthuswamy wrote:
>> Updated the webrev with modified testcase.
>> 
>> webrev: http://cr.openjdk.java.net/~pmuthuswamy/8186807/webrev.01/
>> 
>> Thanks,
>> Priya
>> On 11/3/2017 3:11 PM, Sundararajan Athijegannathan wrote:
>>> Two suggestions:
>>> 
>>> * You may want to add a check to make sure that the control reached 
>>> func.call method
>>> - there is an assert there but the test does not assert the control reached 
>>> there
>>> 
>>> * class name "func" be changed to "Func" or some such (capitalization of 
>>> class names)
>>> 
>>> -Sundar
>>> 
>>> On 03/11/17, 11:36 AM, Priya Lakshmi Muthuswamy wrote:
 Hi,
 
 Please review JDK-8186807: JSObject gets ScriptFunction when 
 ScriptObjectMirror is expected
 
 JBS: https://bugs.openjdk.java.net/browse/JDK-8186807
 webrev: http://cr.openjdk.java.net/~pmuthuswamy/8186807/webrev.00/
 
 Thanks,
 Priya
>> 

RFR: 8189617: Remove undocumented --print-mem-usage option

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8189617
Webrev: http://cr.openjdk.java.net/~hannesw/8189617/webrev.00/

Thanks,
Hannes

Re: RFR: 8068513: Adding elements to a javascript 'object' (a map) is slow

[Hannes Wallnöfer]

From my tests I concluded that leaving the queue threshold high as to not 
affect ‚ordinary‘ objects is the way to go. But it certainly doesn’t hurt to 
make the value configurable. I uploaded a new webrev that does this with a 
system property called „nashorn.propmap.queue.threshold“. It’s otherwise 
unchanged.

http://cr.openjdk.java.net/~hannesw/8068513/webrev.03/

Hannes


> Am 16.10.2017 um 07:43 schrieb Sundararajan Athijegannathan 
> <sundararajan.athijegannat...@oracle.com>:
> 
> Is it worthwhile adding configurability for QUEUE_THREASHOLD? Not a per 
> context property/option - but even a simple nashorn.* system property perhaps?
> 
> +1 other than that
> 
> -Sundar
> 
> On 13/10/17, 7:55 PM, Hannes Wallnöfer wrote:
>> I uploaded a new webrev, please review:
>> 
>> http://cr.openjdk.java.net/~hannesw/8068513/webrev.02/
>> 
>> Changes to previous webrev:
>> 
>>  - updated to consolidated repository layout
>>  - fixed typos and improved/added some comments
>>  - improved test
>> 
>> Thanks,
>> Hannes
>> 
>> 
>>> Am 11.09.2017 um 16:18 schrieb Hannes 
>>> Wallnöfer<hannes.wallnoe...@oracle.com>:
>>> 
>>> Unfortunately I rushed the first webrev a bit, and a couple of bugs slipped 
>>> in.
>>> 
>>>  - PropertyHashMap(MapBuilder) constructor checks its own bins field 
>>> instead of MapBuilder’s for calculating threshold
>>>  - ElementQueue.cloneAndMerge() updates the queue field in PropertyHashMap 
>>> instead of just returning cloned and merged bins
>>> 
>>> I uploaded a new webrev that fixes these problems, everything I wrote in my 
>>> original RFR still applies.
>>> 
>>> http://cr.openjdk.java.net/~hannesw/8068513/webrev.01/
>>> 
>>> Thanks,
>>> Hannes
>>> 
>>> 
>>>> Am 05.09.2017 um 19:57 schrieb Hannes 
>>>> Wallnöfer<hannes.wallnoe...@oracle.com>:
>>>> 
>>>> Please review 8068513: Adding elements to a javascript 'object' (a map) is 
>>>> slow:
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8068513
>>>> Webrev: http://cr.openjdk.java.net/~hannesw/8068513/webrev.00/
>>>> 
>>>> This adds a new singly linked list called ‚ElementQueue‘ to 
>>>> PropertyHashMap that is used above a certain map size to store newly 
>>>> inserted elements without having to hash them (and therefore clone the 
>>>> bins array) immediately. Instead, The queue is merged into the hash bins 
>>>> at certain intervals, either every 512th insertions, or when a map's queue 
>>>> is searched for properties more than a few times.
>>>> 
>>>> Merging the queue every 512 insertions proved to be the best balance 
>>>> between keeping the list searchable (we still need to check it for 
>>>> duplicates when adding elements) and avoiding too frequent cloning.
>>>> 
>>>> In order to merge the queue to optimise query performance, the queue field 
>>>> needs to be non-final. To preserve thread safety, ElementQueue bundles 
>>>> both the bins and queue components, so it can replace both with the update 
>>>> of a single reference in PropertyHashMap. The old and new ElementQueue 
>>>> instances logically contain the same elements, so it is safe for other 
>>>> threads to keep using the old instance. I was thinking of maybe making the 
>>>> queue field volatile, but I don’t think this should be an issue.
>>>> 
>>>> As part of this change I also added a new MapBuilder class that helps 
>>>> derive new maps from the existing ones by adding, replacing, or removing 
>>>> elements. The code is a bit more complex now with three possible storage 
>>>> data structures (list, bins, queue), but it’s still not too bad.
>>>> 
>>>> I made sure that the code used for maps beneath the queue threshold is 
>>>> largely the same as before. Performance of the new combined behaveior is 
>>>> very close to before. The queued implementation itself performs pretty 
>>>> close to the normal implementation (apart from insertion on large maps of 
>>>> course) - I tested much lower thresholds during development, and it was 
>>>> still very good.
>>>> 
>>>> Of course, all tests pass and performance is comparable or maybe slightly 
>>>> faster for some code.
>>>> 
>>>> Thanks,
>>>> Hannes

Re: RFR: 8068513: Adding elements to a javascript 'object' (a map) is slow

[Hannes Wallnöfer]

I uploaded a new webrev, please review:

http://cr.openjdk.java.net/~hannesw/8068513/webrev.02/

Changes to previous webrev:

 - updated to consolidated repository layout
 - fixed typos and improved/added some comments
 - improved test

Thanks,
Hannes


> Am 11.09.2017 um 16:18 schrieb Hannes Wallnöfer 
> <hannes.wallnoe...@oracle.com>:
> 
> Unfortunately I rushed the first webrev a bit, and a couple of bugs slipped 
> in.
> 
>  - PropertyHashMap(MapBuilder) constructor checks its own bins field instead 
> of MapBuilder’s for calculating threshold
>  - ElementQueue.cloneAndMerge() updates the queue field in PropertyHashMap 
> instead of just returning cloned and merged bins
> 
> I uploaded a new webrev that fixes these problems, everything I wrote in my 
> original RFR still applies.
> 
> http://cr.openjdk.java.net/~hannesw/8068513/webrev.01/
> 
> Thanks,
> Hannes
> 
> 
>> Am 05.09.2017 um 19:57 schrieb Hannes Wallnöfer 
>> <hannes.wallnoe...@oracle.com>:
>> 
>> Please review 8068513: Adding elements to a javascript 'object' (a map) is 
>> slow:
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8068513
>> Webrev: http://cr.openjdk.java.net/~hannesw/8068513/webrev.00/
>> 
>> This adds a new singly linked list called ‚ElementQueue‘ to PropertyHashMap 
>> that is used above a certain map size to store newly inserted elements 
>> without having to hash them (and therefore clone the bins array) 
>> immediately. Instead, The queue is merged into the hash bins at certain 
>> intervals, either every 512th insertions, or when a map's queue is searched 
>> for properties more than a few times.
>> 
>> Merging the queue every 512 insertions proved to be the best balance between 
>> keeping the list searchable (we still need to check it for duplicates when 
>> adding elements) and avoiding too frequent cloning.
>> 
>> In order to merge the queue to optimise query performance, the queue field 
>> needs to be non-final. To preserve thread safety, ElementQueue bundles both 
>> the bins and queue components, so it can replace both with the update of a 
>> single reference in PropertyHashMap. The old and new ElementQueue instances 
>> logically contain the same elements, so it is safe for other threads to keep 
>> using the old instance. I was thinking of maybe making the queue field 
>> volatile, but I don’t think this should be an issue.
>> 
>> As part of this change I also added a new MapBuilder class that helps derive 
>> new maps from the existing ones by adding, replacing, or removing elements. 
>> The code is a bit more complex now with three possible storage data 
>> structures (list, bins, queue), but it’s still not too bad.
>> 
>> I made sure that the code used for maps beneath the queue threshold is 
>> largely the same as before. Performance of the new combined behaveior is 
>> very close to before. The queued implementation itself performs pretty close 
>> to the normal implementation (apart from insertion on large maps of course) 
>> - I tested much lower thresholds during development, and it was still very 
>> good. 
>> 
>> Of course, all tests pass and performance is comparable or maybe slightly 
>> faster for some code.
>> 
>> Thanks,
>> Hannes
> 

Re: RFR: 8027302: Identifiers containing unicode escapes are not recognized as reserved words

[Hannes Wallnöfer]

Am 13.10.2017 um 14:35 schrieb Sundararajan Athijegannathan 
<sundararajan.athijegannat...@oracle.com>:
> 
> 
>  36 } catch (e if e instanceof SyntaxError) {
>  37 Assert.assertTrue(e instanceof SyntaxError);
>  38 }
>  39
> 
> In test. Do you want to just catch catch(e) ?
> 

Indeed. I forgot I left this in. Thanks for the review!

Hannes

> -Sundar
> 
> On 13/10/17, 2:06 PM, Hannes Wallnöfer wrote:
>> Please review:
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8027302
>> Webrev: http://cr.openjdk.java.net/~hannesw/8027302/webrev.01
>> 
>> ES6 and ES5 require different handling regarding unencoding Unicode escapes 
>> in keywords and identifiers. This patch makes handling more compliant and 
>> more efficient in ES6 mode (previously we didn’t cover all cases, and 
>> checked every identifier instead of just those containing escapes).
>> 
>> About the removal of Lexer.isEscapeCharacter: this method always returns 
>> true in Lexer, and it’s only purpose was to be overridden in JSON parser, 
>> but JSON parser doesn’t extend Lexer/Parser anymore.
>> 
>> Thanks,
>> Hannes

RFR: 8027302: Identifiers containing unicode escapes are not recognized as reserved words

[Hannes Wallnöfer]

Please review: 

Bug: https://bugs.openjdk.java.net/browse/JDK-8027302
Webrev: http://cr.openjdk.java.net/~hannesw/8027302/webrev.01

ES6 and ES5 require different handling regarding unencoding Unicode escapes in 
keywords and identifiers. This patch makes handling more compliant and more 
efficient in ES6 mode (previously we didn’t cover all cases, and checked every 
identifier instead of just those containing escapes).

About the removal of Lexer.isEscapeCharacter: this method always returns true 
in Lexer, and it’s only purpose was to be overridden in JSON parser, but JSON 
parser doesn’t extend Lexer/Parser anymore.

Thanks,
Hannes

Re: RFR:8147076(LinkerCallSite.ARGLIMIT is used incorrectly)

[Hannes Wallnöfer]

Looks good!

Hannes


> Am 29.09.2017 um 17:37 schrieb Srinivas Dama :
> 
> Hi,
> 
> here is the revised patch
> http://cr.openjdk.java.net/~sdama/8147076/webrev.01/ 
> 
> Regards,
> Srinivas
> - Original Message -
> From: hannes.wallnoe...@oracle.com
> To: srinivas.d...@oracle.com
> Cc: nashorn-dev@openjdk.java.net
> Sent: Friday, September 29, 2017 7:33:18 PM GMT +05:30 Chennai, Kolkata, 
> Mumbai, New Delhi
> Subject: Re: RFR:8147076(LinkerCallSite.ARGLIMIT is used incorrectly)
> 
> Hi Srini, 
> 
> a few notes:
> 
> - Shouldn’t the new test include an invocation with >  ARGLIMIT double 
> arguments to test it works?
> - No need to use Number(1.1), a number with a fractional part will result in 
> a double.
> - I assume eval() in the test is to force this-object and callee to be sent, 
> maybe add a comment about that?
> - there’s a typo in the test: arguements
> 
> Hannes
> 
> 
>> Am 29.09.2017 um 15:34 schrieb Srinivas Dama :
>> 
>> Hi,
>> 
>> Please review http://cr.openjdk.java.net/~sdama/8147076/webrev.00/ 
>> for https://bugs.openjdk.java.net/browse/JDK-8147076
>> 
>> Regards,
>> Srinivas
> 

Re: RFR:8147076(LinkerCallSite.ARGLIMIT is used incorrectly)

[Hannes Wallnöfer]

Hi Srini, 

a few notes:

 - Shouldn’t the new test include an invocation with >  ARGLIMIT double 
arguments to test it works?
 - No need to use Number(1.1), a number with a fractional part will result in a 
double.
 - I assume eval() in the test is to force this-object and callee to be sent, 
maybe add a comment about that?
- there’s a typo in the test: arguements

Hannes


> Am 29.09.2017 um 15:34 schrieb Srinivas Dama :
> 
> Hi,
> 
> Please review http://cr.openjdk.java.net/~sdama/8147076/webrev.00/ 
> for https://bugs.openjdk.java.net/browse/JDK-8147076
> 
> Regards,
> Srinivas

Re: RFR 8180274: Fix links in nashorn documentation

[Hannes Wallnöfer]

+1

Hannes


> Am 29.09.2017 um 06:56 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8180274
> Webrev: http://cr.openjdk.java.net/~sundar/8180274/webrev.00/
> 
> Thanks,
> -Sundar

Re: RFR 8188098: NPE in SimpleTreeVisitorES6 visitor when parsing a tagged template literal

[Hannes Wallnöfer]

+1, good catch!

Hannes

> Am 28.09.2017 um 18:38 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8188098
> Webrev: http://cr.openjdk.java.net/~sundar/8188098/webrev.00/
> 
> Thanks,
> -Sundar

Re: RFR 8188082: autoimports.js sample is broken

[Hannes Wallnöfer]

+1

That tagged literal demo is nifty!

Hannes

> Am 28.09.2017 um 07:37 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8188082
> Webrev: http://cr.openjdk.java.net/~sundar/8188082/webrev.00/
> 
> PS. Piggybacking to push another sample to demonstrate the use of es6 tagged 
> template literal to create a java object
> 
> Thanks,
> -Sundar

RFR: 8186815: Java.from has a bug, when element is ScriptObject

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8186815
Webrev: http://cr.openjdk.java.net/~hannesw/8186815/webrev/

Thanks,
Hannes

Re: RFR 8188023: Avoid -source and -target javac options in nashorn ant compilation

[Hannes Wallnöfer]

+1

Hannes


> Am 27.09.2017 um 14:07 schrieb Jim Laskey (Oracle) :
> 
> +1
> 
>> On Sep 27, 2017, at 8:59 AM, Sundararajan Athijegannathan 
>>  wrote:
>> 
>> Please review.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8188023
>> Webrev: http://cr.openjdk.java.net/~sundar/8188023/webrev.00/index.html
>> 
>> Piggybacking to take care of few javac warnings as well.
>> 
>> Thanks,
>> -Sundar
> 

Re: RFR:8133623:JDK-8055034.js and JDK-8130127.js fail in nashorn nightly

[Hannes Wallnöfer]

+1

Hannes

> Am 27.09.2017 um 08:17 schrieb Priya Lakshmi Muthuswamy 
> :
> 
> Hi,
> 
> --patch-module actually works and added it to back to the testcase.
> 
> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8133623/webrev.01/
> 
> Thanks,
> Priya
> 
> On 9/25/2017 11:17 AM, Priya Lakshmi Muthuswamy wrote:
>> Hi,
>> 
>> Please review JDK-8133623 : JDK-8055034.js and JDK-8130127.js fail in 
>> nashorn nightly.
>> 
>> JDK-8130127.js was already fixed.
>> This RFR is for JDK-8055034.js.
>> 
>> JBS:https://bugs.openjdk.java.net/browse/JDK-8133623
>> webrev: http://cr.openjdk.java.net/~pmuthuswamy/8133623/webrev.00/
>> 
>> Thanks,
>> Priya
> 

RFR: 8187962: Optimistic types ignore JavaAdapter return types

[Hannes Wallnöfer]

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8187962
Webrev: http://cr.openjdk.java.net/~hannesw/8187962/webrev/

Thanks,
Hannes

Re: RFR 8187965: dynalink samples under $jdk10/src/sample/nashorn/dynalink are broken

[Hannes Wallnöfer]

+1

Hannes


> Am 26.09.2017 um 16:41 schrieb Jim Laskey (Oracle) :
> 
> +1 nicer too
> 
> 
>> On Sep 26, 2017, at 10:58 AM, Sundararajan Athijegannathan 
>>  wrote:
>> 
>> Please review.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8187965
>> 
>> Webrev: http://cr.openjdk.java.net/~sundar/8187965/webrev.00/
>> 
>> Dynalink samples are broken because of wildcard usage in jar command.
>> 
>> PS. Piggybacking another sample script base64.js
>> 
>> Thanks,
>> -Sundar
> 

Re: RFR: 8185257(Nashorn AST is missing nodes when a for-loop contains a VariableDeclarationList)

[Hannes Wallnöfer]

Looks good!

Hannes


> Am 25.09.2017 um 21:31 schrieb Srinivas Dama <srinivas.d...@oracle.com>:
> 
> Hi,
> 
> Please review revised patch after migrating to jdk10/master repo.
> http://cr.openjdk.java.net/~sdama/8185257/webrev.02/ 
> 
> Regards,
> Srinivas
> -Original Message-
> From: Hannes Wallnöfer 
> Sent: Wednesday, September 13, 2017 1:49 AM
> To: Srinivas Dama
> Cc: Nashorn-dev
> Subject: Re: RFR: 8185257(Nashorn AST is missing nodes when a for-loop 
> contains a VariableDeclarationList)
> 
> +1
> 
> Hannes
> 
>> Am 12.09.2017 um 19:38 schrieb Sundararajan Athijegannathan 
>> <sundararajan.athijegannat...@oracle.com>:
>> 
>> +1
>> 
>> -Sundar
>> 
>> On 12/09/17, 11:02 PM, Srinivas Dama wrote:
>>> Thank you for the comments sundar.
>>> 
>>> Here is the revised patch with test case modified.
>>> http://cr.openjdk.java.net/~sdama/8185257/webrev.01/
>>> 
>>> Regards,
>>> Srinivas
>>> - Original Message -
>>> From: sundararajan.athijegannat...@oracle.com
>>> To: nashorn-dev@openjdk.java.net
>>> Sent: Tuesday, September 12, 2017 6:49:09 PM GMT +05:30 Chennai, 
>>> Kolkata, Mumbai, New Delhi
>>> Subject: Re: RFR: 8185257(Nashorn AST is missing nodes when a 
>>> for-loop contains a VariableDeclarationList)
>>> 
>>> You may want to print Tree kind in the test rather than using 
>>> implementation class name (and using that it .EXPECTED file).
>>> 
>>> Other than that, +1
>>> 
>>> -Sundar
>>> 
>>> On 12/09/17, 4:58 PM, Srinivas Dama wrote:
>>>> Please review http://cr.openjdk.java.net/~sdama/8185257/webrev.00/
>>>> for https://bugs.openjdk.java.net/browse/JDK-8185257
>>>> 
>>>> Regards,
>>>> Srinivas
> 

Re: RFR 8187934: dropping a shebang script in src/sample/nashorn directory results in test failure

[Hannes Wallnöfer]

+1

Hannes

> Am 25.09.2017 um 16:28 schrieb Jim Laskey (Oracle) :
> 
> +1
> 
>> On Sep 25, 2017, at 11:15 AM, Sundararajan Athijegannathan 
>>  wrote:
>> 
>> Please review.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8187934
>> Webrev: http://cr.openjdk.java.net/~sundar/8187934/webrev.00/
>> 
>> PS. Piggybacking a sample script (psgrep.js)
>> 
>> Thanks,
>> -Sundar
> 

Re: RFR:8186011(Fix samples/java_completion.js and samples/disassemble.js)

[Hannes Wallnöfer]

+1

Hannes

> Am 25.09.2017 um 12:49 schrieb Srinivas Dama :
> 
> Hi,
> Please review migrated patch to jdk10/master repo.
> http://cr.openjdk.java.net/~sdama/8186011/webrev.01/
> 
> Regards,
> Srinivas
> - Original Message -
> From: hannes.wallnoe...@oracle.com
> To: srinivas.d...@oracle.com
> Cc: nashorn-dev@openjdk.java.net
> Sent: Tuesday, September 19, 2017 4:10:46 PM GMT +05:30 Chennai, Kolkata, 
> Mumbai, New Delhi
> Subject: Re: RFR:8186011(Fix samples/java_completion.js and 
> samples/disassemble.js)
> 
> +1
> 
> Hannes
> 
> 
>> Am 19.09.2017 um 12:42 schrieb Sundararajan Athijegannathan 
>> :
>> 
>> +1
>> 
>> -Sundar
>> 
>> On 19/09/17, 4:03 PM, Srinivas Dama wrote:
>>> Hi,
>>> 
>>> Please review http://cr.openjdk.java.net/~sdama/8186011/webrev.00/
>>> for https://bugs.openjdk.java.net/browse/JDK-8186011
>>> 
>>> Regards,
>>> Srinivas
> 

Re: RFR:8133623:JDK-8055034.js and JDK-8130127.js fail in nashorn nightly

[Hannes Wallnöfer]

+1

Hannes


> Am 25.09.2017 um 07:47 schrieb Priya Lakshmi Muthuswamy 
> :
> 
> Hi,
> 
> Please review JDK-8133623 : JDK-8055034.js and JDK-8130127.js fail in nashorn 
> nightly.
> 
> JDK-8130127.js was already fixed.
> This RFR is for JDK-8055034.js.
> 
> JBS:https://bugs.openjdk.java.net/browse/JDK-8133623
> webrev: http://cr.openjdk.java.net/~pmuthuswamy/8133623/webrev.00/
> 
> Thanks,
> Priya