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

[Srinivas Dama]

Hi Attila,
Thank you for the comments.

I have resolved the bug as "will-not-fix" and added appropriate comments in the 
bug.
So I am taking back this RFR.

Regards,
Srinivas

- Original Message -
From: szege...@gmail.com
To: srinivas.d...@oracle.com
Cc: nashorn-dev@openjdk.java.net
Sent: Monday, December 11, 2017 4:45:56 PM GMT +05:30 Chennai, Kolkata, Mumbai, 
New Delhi
Subject: Re: RFR: 8134516(Move getInvokeByName and getDynamicInvoker methods 
from Global to Context)

You shouldn't make Global.getContext() public. Context is a security-sensitive 
class, and the only public access to it is supposed to be through 
Context.getContext(), which includes a security check.

I see you have also simply disabled that security check in your patch. I’m 
afraid that’s unacceptable.

You could make the patch much smaller (and avoid security concerns altogether) 
if you kept Global.getInvokeByName and Global.getDynamicInvoker methods as they 
are and have them internally delegate to their new equivalents in Context, 
adding “this” as the last argument as they do so.

This is all in addition to Hannes’ observation with regard to the performance 
implications in his previous e-mail.

Attila.

> On Dec 6, 2017, at 4:47 PM, Srinivas Dama <srinivas.d...@oracle.com> wrote:
> 
> Hi,
> 
> Please review http://cr.openjdk.java.net/~sdama/8134516/webrev.01/ for 
> https://bugs.openjdk.java.net/browse/JDK-8134516 
> 
> Regards,
> Srinivas

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

[Attila Szegedi]

You shouldn't make Global.getContext() public. Context is a security-sensitive 
class, and the only public access to it is supposed to be through 
Context.getContext(), which includes a security check.

I see you have also simply disabled that security check in your patch. I’m 
afraid that’s unacceptable.

You could make the patch much smaller (and avoid security concerns altogether) 
if you kept Global.getInvokeByName and Global.getDynamicInvoker methods as they 
are and have them internally delegate to their new equivalents in Context, 
adding “this” as the last argument as they do so.

This is all in addition to Hannes’ observation with regard to the performance 
implications in his previous e-mail.

Attila.

> On Dec 6, 2017, at 4:47 PM, Srinivas Dama  wrote:
> 
> Hi,
> 
> Please review http://cr.openjdk.java.net/~sdama/8134516/webrev.01/ for 
> https://bugs.openjdk.java.net/browse/JDK-8134516 
> 
> Regards,
> Srinivas

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: 8134516(Move getInvokeByName and getDynamicInvoker methods from Global to Context)

[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