[PROPOSAL][JDK10] Introduce Executable.getParameterType(index)
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
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.htm...
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
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
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
On Oct 20, 2017, at 4:45 AM, Christoph Dreis <christoph.dreis@freenet.de> 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.
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
On Nov 2, 2017, at 1:33 PM, Christoph Dreis <christoph.dreis@freenet.de> 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
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
On Nov 2, 2017, at 3:00 PM, Christoph Dreis <christoph.dreis@freenet.de> 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
On 2017-11-02 23:13, John Rose wrote:
On Nov 2, 2017, at 3:00 PM, Christoph Dreis <christoph.dreis@freenet.de <mailto:christoph.dreis@freenet.de>> 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
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
On Nov 2, 2017, at 4:33 PM, Christoph Dreis <christoph.dreis@freenet.de> 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
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
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
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
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
participants (5)
-
Alan Bateman
-
Christoph Dreis
-
Claes Redestad
-
John Rose
-
Peter Levart