Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Thu, 3 Mar 2022 14:30:34 GMT, Claes Redestad wrote:
> This is also only a partial solution, since the initial array is created and
> injected by the VM on a "root" Constructor/Method, see for example the code
> for new method:
>
> https://github.com/openjdk/jdk/blob/5c187e34a58769a129a0aae9e4937907c9060202/src/hotspot/share/runtime/reflection.cpp#L778
>
> Depending on usage pattern most of the Constructor/Method objects created are
> such root objects (often created in bulk), and only those exposed to user
> code via a copy will benefit from this particular enhancement. Which might be
> a good optimization when getting the same Constructor/Method from the same
> class over and over.
>
> * Can we skip the `.clone()` when propagating these from a root object to
> one we'll expose? We have internal `getShared...` methods for that purpose.
>
> * If we update the JVM code to inject a reference to
> `Class.EMPTY_CLASS_ARRAY` and we can skip the `.clone()`ing as per above we
> should see a decent benefit overall.
I did also wonder to myself off-list if this style of optimization would be
better handled in the JVM were the arrays of arguments are created. If so, I
wouldn't feel comfortable implementing that in HotSpot sources myself.
> src/java.base/share/classes/java/lang/reflect/Executable.java line 59:
>
>> 57:
>> 58: @SuppressWarnings({"rawtypes"})
>> 59: static final TypeVariable[] NO_TYPE_VARS = new TypeVariable[0];
>
> Since you asked me offline about some startup noise from this patch I took a
> look. This line might be another small contributor, since it means we're
> loading `jlr.TypeVariable` on JVM bootstrap. It would be nice if we could
> move this to `TypeVariable`, but we don't support private/package-private
> interface fields for some reason. Shouldn't we though, now that we have
> private interface fields, etc..?
IIRC, Java language-level private interface fields are on the to-do list, but
are not a high priority.
> src/java.base/share/classes/java/lang/reflect/Executable.java line 61:
>
>> 59: static final TypeVariable[] NO_TYPE_VARS = new TypeVariable[0];
>> 60:
>> 61: static final Class[] NO_TYPES = new Class[0];
>
> Less concerning since it doesn't add extra classloading, but I note that we
> have similar constants in a few places, including
> `j.l.Class.EMPTY_CLASS_ARRAY` and `j.l.r.ProxyGenerator.EMPTY_CLASS_ARRAY`.
> Perhaps would make sense to consolidate these into a single place/instance
> (and use consistent names)
A consolidated location like a non-instantiable java.lang.reflect.EmptyHolder
class with static fields? (Exact name subject to bike shedding.)
-
PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote: > Refactoring of Method and Constructor to share a single empty array for > parameters and exceptions as well as type variables. > > Existing core reflection regression tests pass with the change. This is also only a partial solution, since the initial array is created and injected by the VM on a "root" Constructor/Method, see for example the code for new method: https://github.com/openjdk/jdk/blob/5c187e34a58769a129a0aae9e4937907c9060202/src/hotspot/share/runtime/reflection.cpp#L778 Depending on usage pattern most of the Constructor/Method objects created are such root objects (often created in bulk), and only those exposed to user code via a copy will benefit from this particular enhancement. Which might be a good optimization when getting the same Constructor/Method from the same class over and over. - Can we skip the `.clone()` when propagating these from a root object to one we'll expose? We have internal `getShared...` methods for that purpose. - If we update the JVM code to inject a reference to `Class.EMPTY_CLASS_ARRAY` and we can skip the `.clone()`ing as per above we should see a decent benefit overall. - Changes requested by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote: > Refactoring of Method and Constructor to share a single empty array for > parameters and exceptions as well as type variables. > > Existing core reflection regression tests pass with the change. src/java.base/share/classes/java/lang/reflect/Executable.java line 61: > 59: static final TypeVariable[] NO_TYPE_VARS = new TypeVariable[0]; > 60: > 61: static final Class[] NO_TYPES = new Class[0]; Less concerning since it doesn't add extra classloading, but I note that we have similar constants in a few places, including `j.l.Class.EMPTY_CLASS_ARRAY` and `j.l.r.ProxyGenerator.EMPTY_CLASS_ARRAY`. Perhaps would make sense to consolidate these into a single place/instance (and use consistent names) - PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote:
> Refactoring of Method and Constructor to share a single empty array for
> parameters and exceptions as well as type variables.
>
> Existing core reflection regression tests pass with the change.
src/java.base/share/classes/java/lang/reflect/Executable.java line 59:
> 57:
> 58: @SuppressWarnings({"rawtypes"})
> 59: static final TypeVariable[] NO_TYPE_VARS = new TypeVariable[0];
Since you asked me offline about some startup noise from this patch I took a
look. This line might be another small contributor, since it means we're
loading `jlr.TypeVariable` on JVM bootstrap. It would be nice if we could move
this to `TypeVariable`, but we don't support private/package-private interface
fields for some reason. Shouldn't we though, now that we have private interface
fields, etc..?
-
PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote: > Refactoring of Method and Constructor to share a single empty array for > parameters and exceptions as well as type variables. > > Existing core reflection regression tests pass with the change. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote: > Refactoring of Method and Constructor to share a single empty array for > parameters and exceptions as well as type variables. > > Existing core reflection regression tests pass with the change. PS I'll update the copyrights before pushing. - PR: https://git.openjdk.java.net/jdk/pull/7667
RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
Refactoring of Method and Constructor to share a single empty array for parameters and exceptions as well as type variables. Existing core reflection regression tests pass with the change. - Commit messages: - JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method Changes: https://git.openjdk.java.net/jdk/pull/7667/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7667=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8177107 Stats: 15 lines in 3 files changed: 9 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7667.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7667/head:pull/7667 PR: https://git.openjdk.java.net/jdk/pull/7667
