Re: Review request for JDK-8133299: Nashorn Java adapters should not early bind to functions

[Attila Szegedi]

Please review the second take on JDK-8133299 "Nashorn Java adapters should not 
early bind to functions" at 
 for 


I made some further changes:

- while the code would now be able to invoke just about any Nashorn callable 
object as an implementation for an adapter method, I restricted it to 
ScriptFunction only following a discussion with Sundar. The old adapter only 
supported ScriptFunction, so there's no backwards compatibility issue. 
- I have also made the bytecode of the adapter methods much smaller, by 
extracting some oft-repeated bytecode sequences into static methods on 
JavaAdapterServices (wrapThrowable(), unsupported()). 
- I reworked the way setGlobal works, again drastically reducing the bytecode 
size and moving all of the logic into JavaAdapterServices. 
JavaAdapterServices.setGlobal() now sets the necessary global and returns a 
Runnable that when run() will restore the previous global. The bytecode thus no 
longer has any logic for dealing with globals; it just invokes JAS.setGlobal(), 
stores the returned Runnable, and invokes run() on it in finally block. 
- The generator also detects when the INVOKEDYNAMIC dyn:call would need more 
than 255 slots for its parameter list, and introduces a Nashorn-compliant 
(Object callee, Object this, Object[] args) variable arity invocation in its 
place. 
- Added a fairly comprehensive test suite.

With these changes, this work should be complete. 

It’s probably hard to review the changes in JavaAdapterBytecodeGenerator just 
by looking at the diff, as the changes are quite significant, amounting to a 
partial rewrite. It’s probably easier to apply the diff and review the 
resulting new code of the class. A fairly good way to review the effect of 
these changes is to add “-pc” to line 102 of JavaAdapterTest.java and run it 
with and without the rest of the changes applied to see what kind of code is 
being generated.

Attila.

> On Jan 4, 2016, at 2:48 PM, Attila Szegedi  wrote:
> 
> 
> On Mon, Jan 4, 2016 at 5:13 AM, Sundararajan Athijegannathan 
>  wrote:
> I'm yet to look at the code.  Is it possible to use any callable for methods?
> 
> For members of a delegate ScriptObject, yes. The adapter constructors still 
> only accept either a ScriptObject or a ScriptFunction, but if you pass in an 
> object, its members implementing methods can now be arbitrary callables. 
>  
> If so, do we have tests to cover those cases (like JSObject "functions",  
> DynamicMethods etc.)? In particular, sandbox tests to make sure can't get any 
> more privilege by implementing an interface (for eg. binding sensitive Java 
> method as function implementing interface method and making sure it gets 
> SecurityException when interface method is called).
> 
> We generate separate adapter classes for every ProtectionDomain, so I'm quite 
> confident everything is secure, but it's better to not be overconfident when 
> it comes to security… surely we can add such tests. Do you have some go-to 
> sensitive functionality for testing in this fashion?
> 
> FWIW, I actually started writing some tests for other functionality; what I 
> have so far is tests for correct conversion of primitive return values as 
> well as tests for try/finally for restoring old Global being executed when 
> UnsupportedOperationException is thrown. (All tests I wrote pass, so the code 
> as it is now is correct for these.) I wanted to write few more tests, 
> specifically for binding to vararg functions.
> 
> Actually, I realized that there's one deficiency in the current code that 
> wasn't there before my changes, namely since the CALL operation takes 
> explicit callee and this arguments, the adapter code will actually fail to 
> generate a delegate for a method with more than 253 parameters. I might need 
> to introduce a special case for this, to have them packed into an Object 
> array in this case and do a Nashorn vararg invocation…
> 
> Speaking of which, anyone knows why is LinkerCallSite.ARGLIMIT set to 250 and 
> not 253?
> 
> Attila.
>  
> 
> Thanks,
> -Sundar
> 
> 
> On 1/2/2016 8:31 PM, Attila Szegedi wrote:
> Please review JDK-8133299 "Nashorn Java adapters should not early bind to 
> functions" at  for 
> 
> 
> See implementation notes in 
> 
> 
> Also note that this changeset is based on current tip (rev 1584, 
> [da397aea8ada]) and is as such independent of the change sets for JDK-8144917 
> and JDK-8144919 that are still pending review.
> 
> Thanks,
>Attila.
> 
> 

Re: Review request for JDK-8146274: Thread spinning on WeakHashMap.getEntry() with concurrent use of nashorn

[Attila Szegedi]

Excellent. I knew you’re thorough as usual :-)

> On Jan 15, 2016, at 4:36 PM, Hannes Wallnoefer  
> wrote:
> 
> Thanks for the review.
> 
> Just synchronizing WeakPropertyMapSet methods would not be sufficient, as 
> there is a method that returns the keyset, which is then iterated by the 
> calling code. I could have refactored that (adding 4 listener methods to 
> WeakPropertyMapSet that iterate the keyset internally).
> 
> I chose to go the other way based on observing that very few collections are 
> actually copied at runtime, and it's mostly a one-time thing (once the 
> property maps stabilize, so do the listeners).
> 
> Hannes
> 
> Am 2016-01-15 um 16:06 schrieb Attila Szegedi:
>> +1. I presume you considered it and decided that copying the weak maps is 
>> better than sharing WeakPropertyMapSet objects and making their methods 
>> synchronized.
>> 
>>> On Jan 15, 2016, at 12:49 PM, Hannes Wallnoefer 
>>>  wrote:
>>> 
>>> Please review:
>>> 
>>> Webrev: http://cr.openjdk.java.net/~hannesw/8146274/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8146274
>>> 
>>> I was not able to reproduce the issue although I tried my best to follow 
>>> the instructions. However, it is pretty clear that the issue is that the 
>>> cause for the concurrent modification exception is the sharing of  
>>> WeakPropertyMapSets between PropertyMapListeners.
>>> 
>>> Thanks,
>>> Hannes
> 

Re: Review request for JDK-8146274: Thread spinning on WeakHashMap.getEntry() with concurrent use of nashorn

[Hannes Wallnoefer]

Thanks for the review.

Just synchronizing WeakPropertyMapSet methods would not be sufficient, 
as there is a method that returns the keyset, which is then iterated by 
the calling code. I could have refactored that (adding 4 listener 
methods to WeakPropertyMapSet that iterate the keyset internally).


I chose to go the other way based on observing that very few collections 
are actually copied at runtime, and it's mostly a one-time thing (once 
the property maps stabilize, so do the listeners).


Hannes

Am 2016-01-15 um 16:06 schrieb Attila Szegedi:

+1. I presume you considered it and decided that copying the weak maps is 
better than sharing WeakPropertyMapSet objects and making their methods 
synchronized.


On Jan 15, 2016, at 12:49 PM, Hannes Wallnoefer  
wrote:

Please review:

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

I was not able to reproduce the issue although I tried my best to follow the 
instructions. However, it is pretty clear that the issue is that the cause for 
the concurrent modification exception is the sharing of  WeakPropertyMapSets 
between PropertyMapListeners.

Thanks,
Hannes

Re: Review request for JDK-8146274: Thread spinning on WeakHashMap.getEntry() with concurrent use of nashorn

[Attila Szegedi]

+1. I presume you considered it and decided that copying the weak maps is 
better than sharing WeakPropertyMapSet objects and making their methods 
synchronized.

> On Jan 15, 2016, at 12:49 PM, Hannes Wallnoefer 
>  wrote:
> 
> Please review:
> 
> Webrev: http://cr.openjdk.java.net/~hannesw/8146274/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8146274
> 
> I was not able to reproduce the issue although I tried my best to follow the 
> instructions. However, it is pretty clear that the issue is that the cause 
> for the concurrent modification exception is the sharing of  
> WeakPropertyMapSets between PropertyMapListeners.
> 
> Thanks,
> Hannes

Re: Review request for JDK-8146274: Thread spinning on WeakHashMap.getEntry() with concurrent use of nashorn

[Hannes Wallnoefer]

Thanks Michael. Oops, trying to fix typos and adding new ones instead.

Hannes

Am 2016-01-15 um 13:48 schrieb Michael Haupt:

Hi Hannes,

lower-case thumbs up, with two remarks on the doc changes:

-Object.prototype.toString=returns a string representing of this object
+Object.prototype.toString=returns a string representation this object
... should be "... of this object"

Likewise, for
+Function.prototype.toString=returns a string representation this function

Best,

Michael

Am 15.01.2016 um 12:49 schrieb Hannes Wallnoefer 
mailto:hannes.wallnoe...@oracle.com>>:


Please review:

Webrev: http://cr.openjdk.java.net/~hannesw/8146274/ 


Bug: https://bugs.openjdk.java.net/browse/JDK-8146274

I was not able to reproduce the issue although I tried my best to 
follow the instructions. However, it is pretty clear that the issue 
is that the cause for the concurrent modification exception is the 
sharing of  WeakPropertyMapSets between PropertyMapListeners.


Thanks,
Hannes


--

Oracle 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
OracleJava Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, 
Germany
Green Oracle  	Oracle is committed 
to developing practices and products that help protect the environment

Re: Review request for JDK-8146274: Thread spinning on WeakHashMap.getEntry() with concurrent use of nashorn

[Michael Haupt]

Hi Hannes,

lower-case thumbs up, with two remarks on the doc changes:

-Object.prototype.toString=returns a string representing of this object
+Object.prototype.toString=returns a string representation this object
 
... should be "... of this object"

Likewise, for
+Function.prototype.toString=returns a string representation this function

Best,

Michael

> Am 15.01.2016 um 12:49 schrieb Hannes Wallnoefer 
> :
> 
> Please review:
> 
> Webrev: http://cr.openjdk.java.net/~hannesw/8146274/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8146274
> 
> I was not able to reproduce the issue although I tried my best to follow the 
> instructions. However, it is pretty clear that the issue is that the cause 
> for the concurrent modification exception is the sharing of  
> WeakPropertyMapSets between PropertyMapListeners.
> 
> Thanks,
> Hannes

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment

Review request for JDK-8146274: Thread spinning on WeakHashMap.getEntry() with concurrent use of nashorn

[Hannes Wallnoefer]

Please review:

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

I was not able to reproduce the issue although I tried my best to follow 
the instructions. However, it is pretty clear that the issue is that the 
cause for the concurrent modification exception is the sharing of  
WeakPropertyMapSets between PropertyMapListeners.


Thanks,
Hannes

Re: RFR(S): 8145305: fix Nashorn shebang handling on Cygwin

[Hannes Wallnoefer]

+1

Am 2016-01-15 um 11:31 schrieb Michael Haupt:

Dear all,

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

It adapts the test for shebang handling so that it passes on Cygwin. Some 
documentation extension for NativeArray is piggybacked on the change.

Thanks,

Michael

RFR(S): 8145305: fix Nashorn shebang handling on Cygwin

[Michael Haupt]

Dear all,

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

It adapts the test for shebang handling so that it passes on Cygwin. Some 
documentation extension for NativeArray is piggybacked on the change.

Thanks,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment