[PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

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

[PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
Hey,

while discussing JDK-8029019 & JDK-8189266 Claes came up with the idea to
introduce non-copying access to the underlying parameter types of an
Executable [1]. While there is a (for a good reason?) package-private
Method.getSharedParameterTypes() already I thought of an alternative that
doesn't open up the parameterTypes array to the outside world and actually
solves the use-case where we wanted an allocation-free alternative for
Method.getParameterTypes()[index].

Here is a draft of what I'm thinking of:

Executable:
    /**
     * Returns the {@code Class} object that represents the formal
     * parameter type, at the given index, of the executable
     * represented by this object. The given index starts at 0 and
     * represents the declaration order of the parameters.
     *
     *
     * @param index index to look for the parameter type
     * @return the parameter type at the given index for the executable this
object
     * represents
     * @throws IndexOutOfBoundsException if the parameter type
     *      at the given index of the underlying executable could not be
found
     */
    public abstract Class<?> getParameterType(int index);

Method & Constructor:
   /**
     * {@inheritDoc}
     * @throws IndexOutOfBoundsException {@inheritDoc}
     * @since 18.3
     */
    public Class<?> getParameterType(int index) {
        if (index < 0 || index > getParameterCount()) {
            throw new IndexOutOfBoundsException("No parameter found on index
" + index);
        }
        return parameterTypes[index];
    }

As I don't know what the official @since should look like in the future, do
not pay too much attention on that. I'm not happy with the documentation
wording though.

What do you think about that? Is someone willing to sponsor this given it's
considered worthwhile.

Cheers,
Christoph

[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049520.htm
l

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Claes Redestad
Hi Christoph,


On 2017-10-20 13:45, Christoph Dreis wrote:
> Hey,
>
> while discussing JDK-8029019 & JDK-8189266 Claes came up with the idea to
> introduce non-copying access to the underlying parameter types of an
> Executable [1].

I only recalled Peter had a version of his patch for 8029019 that
included making a shared secret out of
Method.getSharedParameterTypes(), which is kept package-private
for good reason (giving anyone outside the module direct access
to shared arrays is never safe - clone defensively!).

A non-intrusive public API to get at the parameter types safely might
be better overall, though, so if you want to give me some credit for
this then I don't mind... :-)

> While there is a (for a good reason?) package-private
> Method.getSharedParameterTypes() already I thought of an alternative that
> doesn't open up the parameterTypes array to the outside world and actually
> solves the use-case where we wanted an allocation-free alternative for
> Method.getParameterTypes()[index].
>
> Here is a draft of what I'm thinking of:
>
> Executable:
>      /**
>       * Returns the {@code Class} object that represents the formal
>       * parameter type, at the given index, of the executable
>       * represented by this object. The given index starts at 0 and
>       * represents the declaration order of the parameters.
>       *
>       *
>       * @param index index to look for the parameter type
>       * @return the parameter type at the given index for the executable this
> object
>       * represents
>       * @throws IndexOutOfBoundsException if the parameter type
>       *      at the given index of the underlying executable could not be
> found
>       */
>      public abstract Class<?> getParameterType(int index);
>
> Method & Constructor:
>     /**
>       * {@inheritDoc}
>       * @throws IndexOutOfBoundsException {@inheritDoc}
>       * @since 18.3
>       */
>      public Class<?> getParameterType(int index) {
>          if (index < 0 || index > getParameterCount()) {
>              throw new IndexOutOfBoundsException("No parameter found on index
> " + index);
>          }

I don't think we need the explicit range check here.

>          return parameterTypes[index];
>      }
>
> As I don't know what the official @since should look like in the future, do
> not pay too much attention on that. I'm not happy with the documentation
> wording though.

I'm sure there are some who can comment on the wording around
here... :-)

>
> What do you think about that? Is someone willing to sponsor this given it's
> considered worthwhile.

Overall this design captures the desire to allow performant access to
parameter types, and experience and experiments show that the JIT fails
to eliminate the cloning more often than not.

This means there may be both internal[1] and possibly external use cases
where using this API is an optimization, and a simple getter like this can
also make some code cleaner. It does this without introducing another
shared secret in the meantime, which would only have limited applicability.

All in all I think it can pull its weight by allowing us to reduce JDK
internal
use of getParameterTypes() alone, thus I'm in favor and can volunteer
to sponsor (this will need a CSR etc..)

Thanks!

/Claes

[1] 30+ uses of {Method|Constructor}.getParameterTypes() in java.base
alone, many of which could be improved with this.  Admittedly a few of
them are getParameterTypes().length != 0 that should be replaced with
getParameterCount() != 0 regardless..

>
> Cheers,
> Christoph
>
> [1]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049520.html
>

Reply | Threaded
Open this post in threaded view
|

RE: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
Hey Claes,

thank you for your comments on my proposal.

> >      public Class<?> getParameterType(int index) {
> >          if (index < 0 || index > getParameterCount()) {
> >              throw new IndexOutOfBoundsException("No parameter found
> > on index " + index);
> >          }
>
> I don't think we need the explicit range check here.

I thought about that, but decided against the specific ArrayIndexOutOfBoundsException which would be thrown if we omit the explicit check. My reasoning behind that was that the API should not expose the fact that it's working with an array underneath. I have no strong feelings against your comment, though. I'd be fine with both solutions.

> All in all I think it can pull its weight by allowing us to reduce JDK internal use
> of getParameterTypes() alone, thus I'm in favor and can volunteer to sponsor
> (this will need a CSR etc..)

Anything I can do to help?

Cheers,
Christoph

Reply | Threaded
Open this post in threaded view
|

RE: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
Hey,

> > >      public Class<?> getParameterType(int index) {
> > >          if (index < 0 || index > getParameterCount()) {
> > >              throw new IndexOutOfBoundsException("No parameter found
> > > on index " + index);
> > >          }

> > I don't think we need the explicit range check here.

> I thought about that, but decided against the specific
> ArrayIndexOutOfBoundsException which would be thrown if we omit the
> explicit check. My reasoning behind that was that the API should not expose
> the fact that it's working with an array underneath. I have no strong feelings
> against your comment, though. I'd be fine with both solutions.

On a second thought, people might expect the ArrayIndexOutOfBoundsException.

And it would behave the same way as getParameterTypes()[index] does currently. So I'd probably in favor of omitting it as you suggested.

Cheers,
Christoph

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Claes Redestad

On 2017-10-23 13:30, Christoph Dreis wrote:
>>> I don't think we need the explicit range check here.
>> I thought about that, but decided against the specific
>> ArrayIndexOutOfBoundsException which would be thrown if we omit the
>> explicit check. My reasoning behind that was that the API should not expose
>> the fact that it's working with an array underneath. I have no strong feelings
>> against your comment, though. I'd be fine with both solutions.
> On a second thought, people might expect the ArrayIndexOutOfBoundsException.
>
> And it would behave the same way as getParameterTypes()[index] does currently. So I'd probably in favor of omitting it as you suggested.

Ok.  Specifying the thrown error as IOOBE and not AIOOBE may be fine,
though.

/Claes

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

John Rose-3
In reply to this post by Christoph Dreis
On Oct 20, 2017, at 4:45 AM, Christoph Dreis <[hidden email]> wrote:
>
> What do you think about that? Is someone willing to sponsor this given it's
> considered worthwhile.

Let's set that up on top of the new unmodifiable lists from List.of.
It will be simpler and more powerful in the end.  Coding with
the new unmodifiable collections is really nice, and we should
encourage it.

So for every old-school access method of the form T[] getFoos(),
let's consider adding List<T> getFooList(), which returns a list
that works like List.of.  If we use a shared-secrets mechanism,
we can even reuse the same List.of code, using a private
API List.ofSharedArray(T[]) which doesn't copy the argument..

— John

P.S.  A couple thoughts on naming:

Ultimately it would be good if we could do overload selection
based on return type, in which case we wouldn't have to change
the name of the T[] access functions, just their return types.

If we *are* going to change names, we might consider a more
modern-looking form for immutable getters, which elides that
grody "get" prefix.  The "get" prefix makes the most sense when
there is a possibility of a corresponding "set" operator, but these
data structures have no setters, since they are unmodifiable.
The "get" prefix is notably absent from the java.lang.invoke
APIs, except where there really are setters present also.
I wish we could decide to adopt that convention more widely.

Reply | Threaded
Open this post in threaded view
|

RE: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
Hey John,

I'm not quite able to make the connection between my proposal to add Executable.getParameterType(index) and your answer.

>> What do you think about that? Is someone willing to sponsor this given it's
>> considered worthwhile.

> Let's set that up on top of the new unmodifiable lists from List.of.
> It will be simpler and more powerful in the end.  Coding with
> the new unmodifiable collections is really nice, and we should
> encourage it.

> So for every old-school access method of the form T[] getFoos(),
> let's consider adding List<T> getFooList(), which returns a list
> that works like List.of.  If we use a shared-secrets mechanism,
> we can even reuse the same List.of code, using a private
> API List.ofSharedArray(T[]) which doesn't copy the argument..

Could you elaborate, please?

Thanks,
Christoph



Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

John Rose-3
On Nov 2, 2017, at 1:33 PM, Christoph Dreis <[hidden email]> wrote:
>
> Could you elaborate, please?

Sure.  It's really quite simple:  Replace the old mutable array
with a modern immutable list.

Executable:
   /**
     * Returns a list of {@code Class} objects that represent the formal
     * parameter types, in declaration order, of the executable
     * represented by this object.  Returns a list of length
     * 0 if the underlying executable takes no parameters.
     *
     * The list is immutable.
     * See <a href="../../util/List.html#immutable">Immutable List Static Factory Methods</a> for details.
     *
     * @return the parameter types for the executable this object
     * represents
     */
   public abstract List<Class<?>> parameterTypes();
   // "getParameterTypeList" if you must but you shouldn't

(etc.)

Your and Claes' proposal to add the index accessor adds
a more complex API point and requires more complex usage
patterns (for-loops).  In short it's very 90's.  Using a List instead
gives interoperability with new APIs like java.util.stream.

Because of the way immutable lists are organized, the sharing
and code quality at least as good as an indexed access method,
if that was a concern.

— John
Reply | Threaded
Open this post in threaded view
|

RE: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
Hey John,

>> Could you elaborate, please?

> Sure.  It's really quite simple:  Replace the old mutable array
> with a modern immutable list.
> ....
> Your and Claes' proposal to add the index accessor adds
> a more complex API point and requires more complex usage
> patterns (for-loops).  In short it's very 90's.  Using a List instead
> gives interoperability with new APIs like java.util.stream.
> Because of the way immutable lists are organized, the sharing
> and code quality at least as good as an indexed access method,
> if that was a concern.

Thanks for the clarification. Actually the more important concern was to get rid of unnecessary allocations that the cloning inside Executable.getParameterTypes() is producing. As Claes mentioned experience and experiments show that JIT's escaping mechanisms fail from time to time on this one. And if you ask me it is not good practice to rely on the compiler optimizations anyhow, but that's maybe just me.

I do like your proposal nonetheless as an additional improvement, but I think it won't achieve the allocation-free part I was aiming for. Correct me if I'm wrong, please.

Cheers,
Christoph

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

John Rose-3
On Nov 2, 2017, at 3:00 PM, Christoph Dreis <[hidden email]> wrote:
>
> Thanks for the clarification. Actually the more important concern was to get rid of unnecessary allocations that the cloning inside Executable.getParameterTypes() is producing. As Claes mentioned experience and experiments show that JIT's escaping mechanisms fail from time to time on this one. And if you ask me it is not good practice to rely on the compiler optimizations anyhow, but that's maybe just me.
>
> I do like your proposal nonetheless as an additional improvement, but I think it won't achieve the allocation-free part I was aiming for. Correct me if I'm wrong, please.

There are two ways it can directly achieve what you are after.

First, if the guts of the jlr.Method can cache the List and return
the same value every time.  Then the legacy API point can use
List::toArray to create the legacy array values.

Second, if the guts of the jlr.Method choose to cache the Class[],
it can still return a List wrapped around the same array, each time,
as long as the List refuses modification.

Either option avoids the O(N) copying and avoids problems
with escape analysis.  The second option has O(1) allocation,
the first does zero allocation.

The first option also allows constant propagation through the
List.of values, something that arrays can never do (until we
introduce frozen arrays).  This bit of magic is one of several
reasons people who program with arrays should try to move
over to lists.  It is enabled by the private annotation @Stable.

— John
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Claes Redestad


On 2017-11-02 23:13, John Rose wrote:

> On Nov 2, 2017, at 3:00 PM, Christoph Dreis
> <[hidden email] <mailto:[hidden email]>> wrote:
>>
>> Thanks for the clarification. Actually the more important concern was
>> to get rid of unnecessary allocations that the cloning inside
>> Executable.getParameterTypes() is producing. As Claes mentioned
>> experience and experiments show that JIT's escaping mechanisms fail
>> from time to time on this one. And if you ask me it is not good
>> practice to rely on the compiler optimizations anyhow, but that's
>> maybe just me.
>>
>> I do like your proposal nonetheless as an additional improvement, but
>> I think it won't achieve the allocation-free part I was aiming for.
>> Correct me if I'm wrong, please.
>
> There are two ways it can directly achieve what you are after.
>
> First, if the guts of the jlr.Method can cache the List and return
> the same value every time.  Then the legacy API point can use
> List::toArray to create the legacy array values.
>
> Second, if the guts of the jlr.Method choose to cache the Class[],
> it can still return a List wrapped around the same array, each time,
> as long as the List refuses modification.
>
> Either option avoids the O(N) copying and avoids problems
> with escape analysis.  The second option has O(1) allocation,
> the first does zero allocation.
>
> The first option also allows constant propagation through the
> List.of values, something that arrays can never do (until we
> introduce frozen arrays).  This bit of magic is one of several
> reasons people who program with arrays should try to move
> over to lists.  It is enabled by the private annotation @Stable.
>
> — John

I like the first option, and it'd be a nice extra to allow consumers to
move forward from a 90s to an early 2000s programming model. :-)

One drawback is that this would be a somewhat larger effort to a
piece of sensitive, legacy code, but my gut feeling is that by moving
over from arrays to immutable Lists we are likely to reduce risk of
certain bugs creeping in.

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

RE: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
In reply to this post by John Rose-3
Hi John,

>> I do like your proposal nonetheless as an additional improvement, but I think it won't achieve the allocation-free part I was aiming for. Correct me if I'm wrong, please.

> There are two ways it can directly achieve what you are after.
> First, if the guts of the jlr.Method can cache the List and return
> the same value every time.  Then the legacy API point can use
> List::toArray to create the legacy array values.
> Second, if the guts of the jlr.Method choose to cache the Class[],
> it can still return a List wrapped around the same array, each time,
> as long as the List refuses modification.

Again - thank you for sharing your thoughts. I really like your first proposal of caching the parameter list. I am bit concerned though that this has a bigger impact on the overall footprint of Method/Executable objects. What are your thoughts on this?

Cheers,
Christoph


Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

John Rose-3
On Nov 2, 2017, at 4:33 PM, Christoph Dreis <[hidden email]> wrote:
>
> this has a bigger impact on the overall footprint of Method/Executable objects. What are your thoughts on this?

The footprint is probably about the same.  Small List.of values
do not contain arrays, and may be smaller than arrays with the
same number of elements, since they do not have a length field.
And, indeed, methods typically have a small number of parameters.

See the type java.lang.util.ImmutableCollections.List2 for an
example of an array-free data structure.

— John
Reply | Threaded
Open this post in threaded view
|

RE: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Christoph Dreis
Hi John,

>> this has a bigger impact on the overall footprint of Method/Executable objects. What are your thoughts on this?

> The footprint is probably about the same.  Small List.of values
> do not contain arrays, and may be smaller than arrays with the
> same number of elements, since they do not have a length field.
> And, indeed, methods typically have a small number of parameters.

Ah, so you would remove the current array field completely and replace it with the immutable List, right?
In that case I said nothing. I was thinking of a field on top.

Cheers,
Christoph

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Alan Bateman
On 03/11/2017 08:11, Christoph Dreis wrote:

> Hi John,
>
>>> this has a bigger impact on the overall footprint of Method/Executable objects. What are your thoughts on this?
>> The footprint is probably about the same.  Small List.of values
>> do not contain arrays, and may be smaller than arrays with the
>> same number of elements, since they do not have a length field.
>> And, indeed, methods typically have a small number of parameters.
> Ah, so you would remove the current array field completely and replace it with the immutable List, right?
> In that case I said nothing. I was thinking of a field on top.
>
The VM creates Method objects and sets the fields, including
parameterTypes, directly so I think removing it would require more work
than it initially looks. If you add a field then it does increase the
footprint a bit. Alternatively, have the method could use a
ImmutableCollection.ListN like implementation that is backed by the
array and doesn't scan it for nulls at create time, this wouldn't be
completely allocation free of course.

In any case, the proposed API does look reasonable although it
deviations from the usual conventions in java.lang.reflect.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Claes Redestad


On 2017-11-03 14:17, Alan Bateman wrote:

> On 03/11/2017 08:11, Christoph Dreis wrote:
>> Hi John,
>>
>>>> this has a bigger impact on the overall footprint of
>>>> Method/Executable objects. What are your thoughts on this?
>>> The footprint is probably about the same.  Small List.of values
>>> do not contain arrays, and may be smaller than arrays with the
>>> same number of elements, since they do not have a length field.
>>> And, indeed, methods typically have a small number of parameters.
>> Ah, so you would remove the current array field completely and
>> replace it with the immutable List, right?
>> In that case I said nothing. I was thinking of a field on top.
>>
> The VM creates Method objects and sets the fields, including
> parameterTypes, directly so I think removing it would require more
> work than it initially looks. If you add a field then it does increase
> the footprint a bit. Alternatively, have the method could use a
> ImmutableCollection.ListN like implementation that is backed by the
> array and doesn't scan it for nulls at create time, this wouldn't be
> completely allocation free of course.

While it would be pretty sweet if we could train the VM to use List.of
as appropriate (might be applicable in a number of places where we want
to communicate immutable array-like data from the VM), it should be
pretty straightforward to change the interaction so that the VM calls a
private method that takes the parameter array and turns it into a List.

My guess is that the superfluous null checking will be hardly measurable
even in micros.

> In any case, the proposed API does look reasonable although it
> deviations from the usual conventions in java.lang.reflect.

I agree the getParameterType(index) method should still up for
consideration if we can't act on the more elegant proposal to turn
things into Lists in a reasonable time-frame.

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

Re: [PROPOSAL][JDK10] Introduce Executable.getParameterType(index)

Peter Levart
In reply to this post by Alan Bateman
Hi,

On 11/03/2017 02:17 PM, Alan Bateman wrote:

> On 03/11/2017 08:11, Christoph Dreis wrote:
>> Hi John,
>>
>>>> this has a bigger impact on the overall footprint of
>>>> Method/Executable objects. What are your thoughts on this?
>>> The footprint is probably about the same.  Small List.of values
>>> do not contain arrays, and may be smaller than arrays with the
>>> same number of elements, since they do not have a length field.
>>> And, indeed, methods typically have a small number of parameters.
>> Ah, so you would remove the current array field completely and
>> replace it with the immutable List, right?
>> In that case I said nothing. I was thinking of a field on top.
>>
> The VM creates Method objects and sets the fields, including
> parameterTypes, directly so I think removing it would require more
> work than it initially looks. If you add a field then it does increase
> the footprint a bit. Alternatively, have the method could use a
> ImmutableCollection.ListN like implementation that is backed by the
> array and doesn't scan it for nulls at create time, this wouldn't be
> completely allocation free of course.

What if the work was done in 2 phases.

Phase 1: Array typed fields are changed to be of type Object, then for
example Method could use the following implementation, VM code has no
changes:

     private Object parameterTypes;

     public List<Class<?>> parameterTypes() {
         return (List<Class<?>>) parameterTypes;
     }

     public Class<?>[] getParameterTypes() {
         return parameterTypes().toArray(new Class<?>[0]);
     }

...etc.

The "root" Method objects are never used directly. Their methods are
never called. When "root" Method objects are copied, the arrays would be
swapped for lists:

     /**
      * Package-private routine (exposed to java.lang.Class via
      * ReflectAccess) which returns a copy of this Method. The copy's
      * "root" field points to this Method.
      */
     Method copy() {
         // This routine enables sharing of MethodAccessor objects
         // among Method objects which refer to the same underlying
         // method in the VM. (All of this contortion is only necessary
         // because of the "accessibility" bit in AccessibleObject,
         // which implicitly requires that new java.lang.reflect
         // objects be fabricated for each reflective call on Class
         // objects.)
         if (this.root != null)
             throw new IllegalArgumentException("Can not copy a non-root
Method");

         Object _ptypes = parameterTypes;
         if (_ptypes instanceof Class<?>[]) {
             parameterTypes = _ptypes = List.ofShared((Class<?>[]) _ptypes);
         }

         Object _etypes = exceptionTypes;
         if (_etypes instanceof Class<?>[]) {
             exceptionTypes = _etypes = List.ofShared((Class<?>[]) _etypes);
         }

         Method res = new Method(clazz, name, _ptypes, returnType,
                                 _etypes, modifiers, slot, signature,
                                 annotations, parameterAnnotations,
annotationDefault);
         res.root = this;
         // Might as well eagerly propagate this if already present
         res.methodAccessor = methodAccessor;
         return res;
     }



Phase 2: VM is changed to inject List(s) instead of arrays and fields ca
be changed to be of type List with swap-conversions removed.


Regards, Peter