Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2016-07-29 Thread David Holmes 

On 29/07/2016 5:55 PM, Leela Mohan wrote:

Hi David,

I understand, Klass types are no longer oops
 but JNIHandles::resolve_non_null() would expose naked oops. In other
words, KlassOops are no longer oops but java.lang.Class objects are.


Yes my mistake - focusing on the wrong aspect. Good catch.

Thanks,
David


Thanks,
Leela

On Thu, Jul 28, 2016 at 10:51 PM, David Holmes > wrote:

Hi Leela,

On 29/07/2016 12:59 PM, Leela Mohan wrote:

I think, change in the file unsafe.cpp is incorrect. (
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )

Below function is accessing naked oops when thread has
transitioned to
"native":

*+ static jobject get_class_loader(JNIEnv* env, jclass cls)
{**+   if
(java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
{**+ return NULL;**+   }**+   Klass* k =
java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
loader = k->class_loader();**+   return JNIHandles::make_local(env,
loader);**+ }*


klass types are no longer oops in the Java heap, but metaspace
objects that reside in a per-classloader allocation region. They
never get compacted or relocated so raw pointers to them are safe.

Cheers,
David



- Leela

On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
coleen.phillim...@oracle.com
> wrote:


Thanks, Mandy!

Coleen


On 9/8/14, 6:59 PM, Mandy Chung wrote:

Thumbs up.

Mandy

On 9/5/2014 12:55 PM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance
for fast access

This is a backport request for 8u40.   This change
has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot
change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link
https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both
jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.
The hotspot change can
be tested separately from the jdk change (but not
the other way around).

Thanks,
Coleen

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2016-07-29 Thread Leela Mohan 
I think, change in the file unsafe.cpp is incorrect. (
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )

Below function is accessing naked oops when thread has transitioned to
"native":

*+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
(java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
{**+ return NULL;**+   }**+   Klass* k =
java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
loader = k->class_loader();**+   return JNIHandles::make_local(env,
loader);**+ }*


- Leela

On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
coleen.phillim...@oracle.com> wrote:

>
> Thanks, Mandy!
>
> Coleen
>
>
> On 9/8/14, 6:59 PM, Mandy Chung wrote:
>
>> Thumbs up.
>>
>> Mandy
>>
>> On 9/5/2014 12:55 PM, Coleen Phillimore wrote:
>>
>>> Summary: Add classLoader to java/lang/Class instance for fast access
>>>
>>> This is a backport request for 8u40.   This change has been in the jdk9
>>> code for 3 months without any problems.
>>>
>>> The JDK changes hg imported cleanly.  The Hotspot change needed a hand
>>> merge for create_mirror call in klass.cpp.
>>>
>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/
>>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>>>
>>> Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
>>> jck java_lang tests with only the hotspot change.  The hotspot change can
>>> be tested separately from the jdk change (but not the other way around).
>>>
>>> Thanks,
>>> Coleen
>>>
>>
>>
>

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2016-07-29 Thread Leela Mohan 
Hi David,

I understand, Klass types are no longer oops  but
JNIHandles::resolve_non_null()
would expose naked oops. In other words, KlassOops are no longer oops but
java.lang.Class objects are.

Thanks,
Leela

On Thu, Jul 28, 2016 at 10:51 PM, David Holmes 
wrote:

> Hi Leela,
>
> On 29/07/2016 12:59 PM, Leela Mohan wrote:
>
>> I think, change in the file unsafe.cpp is incorrect. (
>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )
>>
>> Below function is accessing naked oops when thread has transitioned to
>> "native":
>>
>> *+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
>> (java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
>> {**+ return NULL;**+   }**+   Klass* k =
>> java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
>> loader = k->class_loader();**+   return JNIHandles::make_local(env,
>> loader);**+ }*
>>
>
> klass types are no longer oops in the Java heap, but metaspace objects
> that reside in a per-classloader allocation region. They never get
> compacted or relocated so raw pointers to them are safe.
>
> Cheers,
> David
>
>
>
>> - Leela
>>
>> On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
>> coleen.phillim...@oracle.com> wrote:
>>
>>
>>> Thanks, Mandy!
>>>
>>> Coleen
>>>
>>>
>>> On 9/8/14, 6:59 PM, Mandy Chung wrote:
>>>
>>> Thumbs up.

 Mandy

 On 9/5/2014 12:55 PM, Coleen Phillimore wrote:

 Summary: Add classLoader to java/lang/Class instance for fast access
>
> This is a backport request for 8u40.   This change has been in the jdk9
> code for 3 months without any problems.
>
> The JDK changes hg imported cleanly.  The Hotspot change needed a hand
> merge for create_mirror call in klass.cpp.
>
> http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/
>
> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>
> Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
> jck java_lang tests with only the hotspot change.  The hotspot change
> can
> be tested separately from the jdk change (but not the other way
> around).
>
> Thanks,
> Coleen
>
>


>>>

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2016-07-28 Thread David Holmes 

Hi Leela,

On 29/07/2016 12:59 PM, Leela Mohan wrote:

I think, change in the file unsafe.cpp is incorrect. (
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )

Below function is accessing naked oops when thread has transitioned to
"native":

*+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
(java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
{**+ return NULL;**+   }**+   Klass* k =
java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
loader = k->class_loader();**+   return JNIHandles::make_local(env,
loader);**+ }*


klass types are no longer oops in the Java heap, but metaspace objects 
that reside in a per-classloader allocation region. They never get 
compacted or relocated so raw pointers to them are safe.


Cheers,
David



- Leela

On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
coleen.phillim...@oracle.com> wrote:



Thanks, Mandy!

Coleen


On 9/8/14, 6:59 PM, Mandy Chung wrote:


Thumbs up.

Mandy

On 9/5/2014 12:55 PM, Coleen Phillimore wrote:


Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.  The hotspot change can
be tested separately from the jdk change (but not the other way around).

Thanks,
Coleen

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2014-09-08 Thread Coleen Phillimore 


Thanks David!
Coleen

On 9/7/14, 9:38 PM, David Holmes wrote:

Looks okay to me.

David

On 6/09/2014 5:55 AM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.  The hotspot change
can be tested separately from the jdk change (but not the other way
around).

Thanks,
Coleen

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2014-09-08 Thread Coleen Phillimore 


Thanks, Mandy!

Coleen

On 9/8/14, 6:59 PM, Mandy Chung wrote:

Thumbs up.

Mandy

On 9/5/2014 12:55 PM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the 
jdk9 code for 3 months without any problems.


The JDK changes hg imported cleanly.  The Hotspot change needed a 
hand merge for create_mirror call in klass.cpp.


http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also 
ran jck java_lang tests with only the hotspot change.  The hotspot 
change can be tested separately from the jdk change (but not the 
other way around).


Thanks,
Coleen

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2014-09-07 Thread David Holmes 

Looks okay to me.

David

On 6/09/2014 5:55 AM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.  The hotspot change
can be tested separately from the jdk change (but not the other way
around).

Thanks,
Coleen

[8u40] RFR 6642881: Improve performance of Class.getClassLoader()

2014-09-05 Thread Coleen Phillimore 

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9 
code for 3 months without any problems.


The JDK changes hg imported cleanly.  The Hotspot change needed a hand 
merge for create_mirror call in klass.cpp.


http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran 
jck java_lang tests with only the hotspot change.  The hotspot change 
can be tested separately from the jdk change (but not the other way around).


Thanks,
Coleen

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-24 Thread Alan Bateman 

On 24/06/2014 00:45, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to 
the instances of java/lang/Class.  This change restricts reflection 
from changing access to the classLoader field.  In the spec, 
AccessibleObject.setAccessible() may throw SecurityException if the 
accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881
Having setAccessible throw SecurityException when there isn't a security 
manager set is a bit odd but as you point out, setAccessible is 
specified to allow that. The changes looks okay, although I don't think 
strictly necessary.


-Alan.

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-24 Thread Peter Levart 

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to 
the instances of java/lang/Class.  This change restricts reflection 
from changing access to the classLoader field.  In the spec, 
AccessibleObject.setAccessible() may throw SecurityException if the 
accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen


Hi Coleen,

So hackers are prevented from hacking...

Out of curiosity, what would happen if someone changed the classLoader 
field of some Class object? I guess VM still has it's own notion of the 
class' class loader, right? Only the code that directly uses the 
Class.getClassLoader() (and Unsafe.defineClass0) methods would be 
affected...


While we're at it, does this change in any way affect the GC logic? Will 
GC now navigate the classLoader field during marking but previously 
didn't ? Will this have any GC performance penalty ?


Regards, Peter



On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which 
should go away in 9 but …).
I don't know how to hide the field from reflection. It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that 
hides a field from being found using reflection. It might very 
well be the case that private and final is enough, I haven’t 
thought this through 100%. On the other hand, is there a reason to 
give users access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs 
getClassLoader

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since 
you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that either 
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-24 Thread Frederic Parain 

Hi Coleen,

It seems that there's still a reference to JVM_GetClassLoader
in file jdk/src/share/native/common/check_code.c. The code looks
like dead code, but it would be nice to clean it up.

Thanks,

Fred

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to
the instances of java/lang/Class.  This change restricts reflection from
changing access to the classLoader field.  In the spec,
AccessibleObject.setAccessible() may throw SecurityException if the
accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean)


Only AccessibleObject.java has changes from the previous version of this
change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which should
go away in 9 but …).

I don't know how to hide the field from reflection.  It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that hides
a field from being found using reflection. It might very well be
the case that private and final is enough, I haven’t thought this
through 100%. On the other hand, is there a reason to give users
access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs
getClassLoader
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since
you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that either
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel





--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-24 Thread Coleen Phillimore 


Hi Peter,

On 6/24/14, 4:23 AM, Peter Levart wrote:

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field 
to the instances of java/lang/Class.  This change restricts 
reflection from changing access to the classLoader field.  In the 
spec, AccessibleObject.setAccessible() may throw SecurityException if 
the accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen


Hi Coleen,

So hackers are prevented from hacking...

Out of curiosity, what would happen if someone changed the classLoader 
field of some Class object? I guess VM still has it's own notion of 
the class' class loader, right? Only the code that directly uses the 
Class.getClassLoader() (and Unsafe.defineClass0) methods would be 
affected...


True, Class.getClassLoader() calls would be affected but we may use this 
call for security checks.  I'm not really an expert on this, but we 
thought it wouldn't be safe to allow user access to classLoader.




While we're at it, does this change in any way affect the GC logic? 
Will GC now navigate the classLoader field during marking but 
previously didn't ? Will this have any GC performance penalty ?


I actually ran this through our performance testing and got a good 
improvement in one of the scimark benchmarks for no reason I could 
explain (and lost the results), but none of the other tests were 
affected.  GC would have to mark this new field for full collections but 
not young collections because it's only set during initialization.   I 
wouldn't expect this field to have any negative performance for GC.


Thanks,
Coleen



Regards, Peter



On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which 
should go away in 9 but …).
I don't know how to hide the field from reflection. It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class 
loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that 
hides a field from being found using reflection. It might very 
well be the case that private and final is enough, I haven’t 
thought this through 100%. On the other hand, is there a reason 
to give users access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs 
getClassLoader

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but 
since you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that 
either if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-24 Thread Coleen Phillimore 


Fred,

Thank you for finding this.  Yes, I meant to clean this up with the bug 
to remove JVM_GetClassLoader but I should remove this with this change 
instead, since the other change will be in hotspot only.

Yes, it's dead code.

Thanks!
Coleen

On 6/24/14, 4:41 AM, Frederic Parain wrote:

Hi Coleen,

It seems that there's still a reference to JVM_GetClassLoader
in file jdk/src/share/native/common/check_code.c. The code looks
like dead code, but it would be nice to clean it up.

Thanks,

Fred

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to
the instances of java/lang/Class.  This change restricts reflection from
changing access to the classLoader field.  In the spec,
AccessibleObject.setAccessible() may throw SecurityException if the
accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 




Only AccessibleObject.java has changes from the previous version of this
change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which should
go away in 9 but …).

I don't know how to hide the field from reflection. It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class 
loader.



There is sun.reflect.Reflection.registerFieldsToFilter() that hides
a field from being found using reflection. It might very well be
the case that private and final is enough, I haven’t thought this
through 100%. On the other hand, is there a reason to give users
access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs
getClassLoader
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since
you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that either
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-24 Thread Coleen Phillimore 


On 6/24/14, 4:41 AM, Frederic Parain wrote:

Hi Coleen,

It seems that there's still a reference to JVM_GetClassLoader
in file jdk/src/share/native/common/check_code.c. The code looks
like dead code, but it would be nice to clean it up.


I removed this code.  There are no other instances of the macro 
BROKEN_JAVAC.  I update the copyrights when I commit the changeset.


http://cr.openjdk.java.net/~coleenp/6642881_jdk_5/

Thanks!
Coleen


Thanks,

Fred

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to
the instances of java/lang/Class.  This change restricts reflection from
changing access to the classLoader field.  In the spec,
AccessibleObject.setAccessible() may throw SecurityException if the
accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 




Only AccessibleObject.java has changes from the previous version of this
change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which should
go away in 9 but …).

I don't know how to hide the field from reflection. It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class 
loader.



There is sun.reflect.Reflection.registerFieldsToFilter() that hides
a field from being found using reflection. It might very well be
the case that private and final is enough, I haven’t thought this
through 100%. On the other hand, is there a reason to give users
access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs
getClassLoader
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since
you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that either
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-23 Thread Coleen Phillimore 


Please review a change to the JDK code for adding classLoader field to 
the instances of java/lang/Class.  This change restricts reflection from 
changing access to the classLoader field.  In the spec, 
AccessibleObject.setAccessible() may throw SecurityException if the 
accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean)

Only AccessibleObject.java has changes from the previous version of this 
change.


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which should 
go away in 9 but …).
I don't know how to hide the field from reflection.  It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that hides 
a field from being found using reflection. It might very well be 
the case that private and final is enough, I haven’t thought this 
through 100%. On the other hand, is there a reason to give users 
access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs 
getClassLoader

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since 
you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that either 
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-23 Thread Mandy Chung 

Coleen,

On 6/23/2014 4:45 PM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to 
the instances of java/lang/Class.  This change restricts reflection 
from changing access to the classLoader field.  In the spec, 
AccessibleObject.setAccessible() may throw SecurityException if the 
accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.




This change looks reasonable.As a side note:  Joel mentions about 
the mechanism to hide class members from reflection.   I discussed with 
Joel offline before he went on parental leave and suggest that we should 
revisit the two mechanisms that both effectively disallow access to 
private members in the future.


Mandy


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which 
should go away in 9 but …).
I don't know how to hide the field from reflection. It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that 
hides a field from being found using reflection. It might very 
well be the case that private and final is enough, I haven’t 
thought this through 100%. On the other hand, is there a reason to 
give users access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs 
getClassLoader

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since 
you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that either 
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-23 Thread Coleen Phillimore 


On 6/23/14, 9:36 PM, Mandy Chung wrote:

Coleen,

On 6/23/2014 4:45 PM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field 
to the instances of java/lang/Class.  This change restricts 
reflection from changing access to the classLoader field.  In the 
spec, AccessibleObject.setAccessible() may throw SecurityException if 
the accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.




This change looks reasonable.As a side note:  Joel mentions about 
the mechanism to hide class members from reflection.   I discussed 
with Joel offline before he went on parental leave and suggest that we 
should revisit the two mechanisms that both effectively disallow 
access to private members in the future.


Thanks, Mandy.  Yes, let me know what you come up with and we can 
improve this.  Thank you for the help fixing this bug.


Coleen



Mandy


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which 
should go away in 9 but …).
I don't know how to hide the field from reflection. It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class 
loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that 
hides a field from being found using reflection. It might very 
well be the case that private and final is enough, I haven’t 
thought this through 100%. On the other hand, is there a reason 
to give users access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs 
getClassLoader

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but 
since you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that 
either if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

2014-06-23 Thread David Holmes 

On 24/06/2014 11:44 AM, Coleen Phillimore wrote:


On 6/23/14, 9:36 PM, Mandy Chung wrote:

Coleen,

On 6/23/2014 4:45 PM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field
to the instances of java/lang/Class.  This change restricts
reflection from changing access to the classLoader field.  In the
spec, AccessibleObject.setAccessible() may throw SecurityException if
the accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean)


Only AccessibleObject.java has changes from the previous version of
this change.



This change looks reasonable.As a side note:  Joel mentions about
the mechanism to hide class members from reflection.   I discussed
with Joel offline before he went on parental leave and suggest that we
should revisit the two mechanisms that both effectively disallow
access to private members in the future.


Thanks, Mandy.  Yes, let me know what you come up with and we can
improve this.  Thank you for the help fixing this bug.


Note that completely hiding something from reflection is different to 
controlling its accessability via reflection. I think what is being done 
here is the right way to go.


The changes look okay to me.

Thanks,
David



Coleen



Mandy


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which
should go away in 9 but …).

I don't know how to hide the field from reflection. It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class
loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that
hides a field from being found using reflection. It might very
well be the case that private and final is enough, I haven’t
thought this through 100%. On the other hand, is there a reason
to give users access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs
getClassLoader
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but
since you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that
either if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel