RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Paul Sandoz
Hi,

Please review:

  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>

There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.

Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.

The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).

Thanks,
Paul.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Claes Redestad
Hi Paul,

it seems you'll effectively skip processing of the last interface of c in the new code - is this intentional?

3049         Field[] iFields = null;
3050         for (Class<?> c : getInterfaces()) {
3051             if (iFields != null && iFields.length > 0) {
...
3060             }
3061             iFields = c.privateGetPublicFields();
3062         }

ifaces is unused:

3047         Class<?>[] ifaces = getInterfaces();

Nit: You could probably simplify code by replacing List<Field> fields with the LinkedHashSet directly, removing the need to create it conditionally.

/Claes


On 2017-11-29 21:15, Paul Sandoz wrote:

> Hi,
>
> Please review:
>
>    http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>
>
> There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.
>
> Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.
>
> The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).
>
> Thanks,
> Paul.

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Paul Sandoz
Hi Caes,

As we discussed off list the post loop logic is easily missed.

However, i found another obvious issue i missed with two classes (super/sub) extending from the same interface that declares a field. See updated test case in the webrev.

I have an idea to retain Field[] arrays and then process ‘em all at the end of the method to produce the final array. That should hopefully make the logic clearer too.

Paul.


> On 29 Nov 2017, at 16:00, Claes Redestad <[hidden email]> wrote:
>
> Hi Paul,
>
> it seems you'll effectively skip processing of the last interface of c in the new code - is this intentional?
>
> 3049         Field[] iFields = null;
> 3050         for (Class<?> c : getInterfaces()) {
> 3051             if (iFields != null && iFields.length > 0) {
> ...
> 3060             }
> 3061             iFields = c.privateGetPublicFields();
> 3062         }
>
> ifaces is unused:
>
> 3047         Class<?>[] ifaces = getInterfaces();
>
> Nit: You could probably simplify code by replacing List<Field> fields with the LinkedHashSet directly, removing the need to create it conditionally.
>
> /Claes
>
>
> On 2017-11-29 21:15, Paul Sandoz wrote:
>> Hi,
>>
>> Please review:
>>
>>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>
>>
>> There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.
>>
>> Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.
>>
>> The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).
>>
>> Thanks,
>> Paul.
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Paul Sandoz
Here is the updated webrev:

  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/

I opted for the simple solution using a LinkedHashSet.

It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.

Paul.

[*] when there are two or more super interface with fields, or one or more super interfaces and a super classes with fields.


> On 30 Nov 2017, at 08:41, Paul Sandoz <[hidden email]> wrote:
>
> Hi Caes,
>
> As we discussed off list the post loop logic is easily missed.
>
> However, i found another obvious issue i missed with two classes (super/sub) extending from the same interface that declares a field. See updated test case in the webrev.
>
> I have an idea to retain Field[] arrays and then process ‘em all at the end of the method to produce the final array. That should hopefully make the logic clearer too.
>
> Paul.
>
>
>> On 29 Nov 2017, at 16:00, Claes Redestad <[hidden email]> wrote:
>>
>> Hi Paul,
>>
>> it seems you'll effectively skip processing of the last interface of c in the new code - is this intentional?
>>
>> 3049         Field[] iFields = null;
>> 3050         for (Class<?> c : getInterfaces()) {
>> 3051             if (iFields != null && iFields.length > 0) {
>> ...
>> 3060             }
>> 3061             iFields = c.privateGetPublicFields();
>> 3062         }
>>
>> ifaces is unused:
>>
>> 3047         Class<?>[] ifaces = getInterfaces();
>>
>> Nit: You could probably simplify code by replacing List<Field> fields with the LinkedHashSet directly, removing the need to create it conditionally.
>>
>> /Claes
>>
>>
>> On 2017-11-29 21:15, Paul Sandoz wrote:
>>> Hi,
>>>
>>> Please review:
>>>
>>>  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>
>>>
>>> There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.
>>>
>>> Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.
>>>
>>> The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).
>>>
>>> Thanks,
>>> Paul.
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Claes Redestad
Hi,

as expected this is quite a bit easier to follow. Thanks!

As any heavy use of reflection is likely to hit cached data, then
heavily optimizing might be ill-advised here.

A simpler optimization might be to check if the class has any superclass
or interfaces whatsoever first, since if not then publicFields ==
privateGetDeclaredField(true). This might reduce number of
LinkedHashSets created for many trivial class hierarchies significantly
for only a nominal increase in code complexity, and actually reduce the
retained set a bit.

/Claes


On 2017-11-30 21:17, Paul Sandoz wrote:

> Here is the updated webrev:
>
>    http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/
>
> I opted for the simple solution using a LinkedHashSet.
>
> It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.
>
> Paul.
>
> [*] when there are two or more super interface with fields, or one or more super interfaces and a super classes with fields.
>
>
>> On 30 Nov 2017, at 08:41, Paul Sandoz <[hidden email]> wrote:
>>
>> Hi Caes,
>>
>> As we discussed off list the post loop logic is easily missed.
>>
>> However, i found another obvious issue i missed with two classes (super/sub) extending from the same interface that declares a field. See updated test case in the webrev.
>>
>> I have an idea to retain Field[] arrays and then process ‘em all at the end of the method to produce the final array. That should hopefully make the logic clearer too.
>>
>> Paul.
>>
>>
>>> On 29 Nov 2017, at 16:00, Claes Redestad <[hidden email]> wrote:
>>>
>>> Hi Paul,
>>>
>>> it seems you'll effectively skip processing of the last interface of c in the new code - is this intentional?
>>>
>>> 3049         Field[] iFields = null;
>>> 3050         for (Class<?> c : getInterfaces()) {
>>> 3051             if (iFields != null && iFields.length > 0) {
>>> ...
>>> 3060             }
>>> 3061             iFields = c.privateGetPublicFields();
>>> 3062         }
>>>
>>> ifaces is unused:
>>>
>>> 3047         Class<?>[] ifaces = getInterfaces();
>>>
>>> Nit: You could probably simplify code by replacing List<Field> fields with the LinkedHashSet directly, removing the need to create it conditionally.
>>>
>>> /Claes
>>>
>>>
>>> On 2017-11-29 21:15, Paul Sandoz wrote:
>>>> Hi,
>>>>
>>>> Please review:
>>>>
>>>>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>
>>>>
>>>> There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.
>>>>
>>>> Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.
>>>>
>>>> The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).
>>>>
>>>> Thanks,
>>>> Paul.

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Paul Sandoz


> On 30 Nov 2017, at 13:00, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> as expected this is quite a bit easier to follow. Thanks!
>
> As any heavy use of reflection is likely to hit cached data, then heavily optimizing might be ill-advised here.
>
> A simpler optimization might be to check if the class has any superclass or interfaces whatsoever first, since if not then publicFields == privateGetDeclaredField(true). This might reduce number of LinkedHashSets created for many trivial class hierarchies significantly for only a nominal increase in code complexity, and actually reduce the retained set a bit.
>

I am resisting the temptation to do that right now, i threw away a highly optimal implementation, i liked it, but you might not :-) An intermediate solution is to create a pre-sized Field[][] and not be as smart about that allocation.

Mandy is gonna take a careful look next week.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Claes Redestad


On 2017-11-30 23:25, Paul Sandoz wrote:

>
>> On 30 Nov 2017, at 13:00, Claes Redestad <[hidden email]> wrote:
>>
>> Hi,
>>
>> as expected this is quite a bit easier to follow. Thanks!
>>
>> As any heavy use of reflection is likely to hit cached data, then heavily optimizing might be ill-advised here.
>>
>> A simpler optimization might be to check if the class has any superclass or interfaces whatsoever first, since if not then publicFields == privateGetDeclaredField(true). This might reduce number of LinkedHashSets created for many trivial class hierarchies significantly for only a nominal increase in code complexity, and actually reduce the retained set a bit.
>>
> I am resisting the temptation to do that right now, i threw away a highly optimal implementation, i liked it, but you might not :-) An intermediate solution is to create a pre-sized Field[][] and not be as smart about that allocation.

Joel points out the obvious fact that the only class that doesn't have a
superclass is Object, so my optimization idea wouldn't be very effective
without also checking that the super class isn't Object, making it less
sweet as a middle-ground option.

Any optimization should probably be done as a follow-up, anyhow. The
current patch looks fine as-is to me.

>
> Mandy is gonna take a careful look next week.

Yes, I'd not count my review as a capital-R review in this area. Too
much history. :-)

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Mandy Chung
In reply to this post by Paul Sandoz


On 11/30/17 12:17 PM, Paul Sandoz wrote:
> Here is the updated webrev:
>
>    http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/
>
> I opted for the simple solution using a LinkedHashSet.

This fix looks good except a typo in the test:

   94  "Class %s does not have extracly one field: %d", c.getName(), nfs));

s/extracly/exactly/

I am wondering if these @run should run with both othervm and agentvm mode since
it currently depends on the jtreg command-line (I believe our test target uses
agentvm as the default).

> It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.

I was tempted to come up with optimization too when first reading the
patch but I am with you.  I like the resulting code simple and clear.  
The difference is an array list vs linked hash set which we should wait
until this is really a performance issue.  FWIW getMethods
implementation also creates a linked hash set if not cached.

Mandy

> Paul.
>
> [*] when there are two or more super interface with fields, or one or more super interfaces and a super classes with fields.
>
>
>> On 30 Nov 2017, at 08:41, Paul Sandoz <[hidden email]> wrote:
>>
>> Hi Caes,
>>
>> As we discussed off list the post loop logic is easily missed.
>>
>> However, i found another obvious issue i missed with two classes (super/sub) extending from the same interface that declares a field. See updated test case in the webrev.
>>
>> I have an idea to retain Field[] arrays and then process ‘em all at the end of the method to produce the final array. That should hopefully make the logic clearer too.
>>
>> Paul.
>>
>>
>>> On 29 Nov 2017, at 16:00, Claes Redestad <[hidden email]> wrote:
>>>
>>> Hi Paul,
>>>
>>> it seems you'll effectively skip processing of the last interface of c in the new code - is this intentional?
>>>
>>> 3049         Field[] iFields = null;
>>> 3050         for (Class<?> c : getInterfaces()) {
>>> 3051             if (iFields != null && iFields.length > 0) {
>>> ...
>>> 3060             }
>>> 3061             iFields = c.privateGetPublicFields();
>>> 3062         }
>>>
>>> ifaces is unused:
>>>
>>> 3047         Class<?>[] ifaces = getInterfaces();
>>>
>>> Nit: You could probably simplify code by replacing List<Field> fields with the LinkedHashSet directly, removing the need to create it conditionally.
>>>
>>> /Claes
>>>
>>>
>>> On 2017-11-29 21:15, Paul Sandoz wrote:
>>>> Hi,
>>>>
>>>> Please review:
>>>>
>>>>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>
>>>>
>>>> There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.
>>>>
>>>> Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.
>>>>
>>>> The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).
>>>>
>>>> Thanks,
>>>> Paul.

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Paul Sandoz


> On 1 Dec 2017, at 12:16, mandy chung <[hidden email]> wrote:
>
>
>
> On 11/30/17 12:17 PM, Paul Sandoz wrote:
>> Here is the updated webrev:
>>
>>
>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/
>>
>>
>> I opted for the simple solution using a LinkedHashSet.
>>
>
> This fix looks good except a typo in the test:
>
>   94  "Class %s does not have extracly one field: %d", c.getName(), nfs));
>
> s/extracly/exactly/
>

Fixed.


> I am wondering if these @run should run with both othervm and agentvm mode since
> it currently depends on the jtreg command-line


> (I believe our test target uses
> agentvm as the default).
>

It does.

* @run main/othervm StaticFieldsOnInterface C
* @run main/othervm StaticFieldsOnInterface D
* @run main/othervm StaticFieldsOnInterface Y

This ok?


>> It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.
>>
>
> I was tempted to come up with optimization too when first reading the patch but I am with you.  I like the resulting code simple and clear.   The difference is an array list vs linked hash set which we should wait until this is really a performance issue.  FWIW getMethods implementation also creates a linked hash set if not cached.
>

Ok, thanks,
Paul.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

Mandy Chung

> On Dec 1, 2017, at 2:52 PM, Paul Sandoz <[hidden email]> wrote:
>
>
>
>> On 1 Dec 2017, at 12:16, mandy chung <[hidden email]> wrote:
>>
>>
>>
>>> On 11/30/17 12:17 PM, Paul Sandoz wrote:
>>> Here is the updated webrev:
>>>
>>>
>>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/
>>>
>>>
>>> I opted for the simple solution using a LinkedHashSet.
>>>
>>
>> This fix looks good except a typo in the test:
>>
>>  94  "Class %s does not have extracly one field: %d", c.getName(), nfs));
>>
>> s/extracly/exactly/
>>
>
> Fixed.
>
>
>> I am wondering if these @run should run with both othervm and agentvm mode since
>> it currently depends on the jtreg command-line
>
>
>> (I believe our test target uses
>> agentvm as the default).
>>
>
> It does.
>
> * @run main/othervm StaticFieldsOnInterface C
> * @run main/othervm StaticFieldsOnInterface D
> * @run main/othervm StaticFieldsOnInterface Y
>
> This ok?
>

Yes.

Mandy

>
>>> It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.
>>>
>>
>> I was tempted to come up with optimization too when first reading the patch but I am with you.  I like the resulting code simple and clear.   The difference is an array list vs linked hash set which we should wait until this is really a performance issue.  FWIW getMethods implementation also creates a linked hash set if not cached.
>>
>
> Ok, thanks,
> Paul.