Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a Field type in reflect-config.json that Includes all Type's Metadata in the Image #7962

Closed
vjovanov opened this issue Dec 5, 2023 · 8 comments

Comments

@vjovanov
Copy link
Member

vjovanov commented Dec 5, 2023

TL;DR

We'd like to include all the type's metadata for types that are registered for reflection as the default behavior in reflect-config.json. The newly included metadata would contain method signatures, inner classes, field metadata, record components, signers, and permitted subclasses. The transitively included classes would be included in the image, but not registered for reflection.

This is inspired by #7753 where we show that roughly 25% of registration entries can be removed when we use the proposed approach here.

Since this is a breaking change, we propose a new schema for inclusion:

{
    "type": "app.App"
}

as this is anyhow required for proxies and lambdas. The new entry would be equivalent to

{
    "name": "app.App",
    "queryAllDeclaredMethods": true,
    "queryAllDeclaredConstructors": true,
    "queryAllDeclaredFields": true,
    "queryAllPublicMethods": true,
    "queryAllPublicConstructors": true,
    "queryAllPublicFields": true, 
    "allDeclaredClasses": true,
    "allPublicClasses": true,
    "allRecordComponents": true,
    "allNestMembers": true,
    "allSigners": true,
    "allPermittedSubclasses": true
}

It should be possible to disable the metadata from a type by disabling certain properties. For example, we can exclude method metadata with:

{
    "type": "app.App",
    "queryAllDeclaredMethods": false,
    "queryAllPublicMethods": false
}

Since this change will propagate quickly through the ecosystem, we intend to backport this change to GraalVM for JDK 17 and JDK 21.

The format with name will be kept functional in the foreseeable future due to the number of existing projects that use it. However, the agent will be changed to produce the format with type.

Goals

  1. Reduce the load of maintaining reachability metadata.
  2. Allow the development of a programming model aimed to include metadata from user code.
  3. Simplify reasoning about reflection metadata.

Non-Goals

  1. Increase image size significantly--10% or more.
  2. Reduce any of the other performance metrics for Native Image.

Discussion points

  1. Should Class.forName("literal.Type") include all class metadata in the image? Likely not, this would make the programming model unpredictable and bloat the image.

  2. Should we include the default constructors with the type? Likely not, although it would reduce the amount of metadata by 10%.

  3. When merging old and new configs, should the agent (or native-image-configure) subsume the previous config with type? This is necessary as type is always stronger than name or type with some elements disabled.

@vjovanov vjovanov changed the title Include all Type's Metadata by Default for Types that are Registered for Reflection Introduce a Field type in reflect-config.json that Includes all Type's Metadata in the Image Dec 5, 2023
@zakkak
Copy link
Collaborator

zakkak commented Dec 5, 2023

The format with name will be kept functional for a long time due to the number of existing projects that use it. However, the agent will be changed to produce the format with type.

This sounds like name will be deprecated and eventually marked for removal. Is that correct?

Furthermore, I don't see what's the motivation behind having the agent produce the type format. The type format sounds to me like a shortcut/easier way for users to define the metadata they need (when doing so manually). Since the agent is a tool, there is no added difficulty in having it generate the more verbose name-based configuration.
Could you please elaborate a bit more on this?

At least I would expect the agent to have an option to choose between the two alternatives.

Should Class.forName("literal.Type") include all class metadata in the image?

In Quarkus we would like to have the option to not include all class metadata in such cases, be it through keeping the current behavior or through a new option that will define the behavior.

Should we include the default constructors?

Is this question specific to type? If so, I would say yes.

When merging old and new configs, should the agent (or native-image-configure) subsume the previous config with type?

No, if I get it right, this would prevent users, frameworks, and libraries from having finer control on their configuration.

Should we allow to exclude stuff from the type that was included via "type" (e.g., "allDeclaredMethods": false?

Yes, that would be a good way to allow users to optimize their images. Starting with type they get a functional native image without a lot of hastle and they can then proceed to better understand how each type is being accessed and optimize their configuration accordingly.

Non-Goals: Increase image size significantly--10% or more.

Although an image size increase of ~5% may not sound dramatic, we have observed [1][2] such kind of increases in both 23.0 and 23.1. These kind of changes although small on their own, they end up adding up. As a result, we would prefer such kind of changes to be optional (either opt-in or opt-out) when they are not affecting correctness.

[1] https://quarkus.io/blog/mandrel-23-0-image-size-increase/
[2] https://quarkus.io/blog/mandrel-23-1-image-size-increase/

@mhalbritter
Copy link

Should we include the default constructors?

I'd not include the constructors for invocation. I think the mental model should be: when using "type", everything can be queried, but if I want invocation, I need to explicitly opt-in into that.

@vjovanov
Copy link
Member Author

vjovanov commented Dec 6, 2023

@zakkak thanks for your feedback, I will adjust the proposal according to some of the answers.

This sounds like name will be deprecated and eventually marked for removal. Is that correct?

That is not decided and it very much depends on the community adoption. It seems strange that we will have two ways to achieve the same thing.

Since the agent is a tool, there is no added difficulty in having it generate the more verbose name-based configuration. Could you please elaborate a bit more on this?

The goal is to shift the community to reason in terms of complete types. If the agent keeps producing the name format it will be hard to achieve that.
People often use the agent as the first step, but then they manually edit the config. In that case, I would prefer to have type as a starting point.

@zakkak
Copy link
Collaborator

zakkak commented Dec 6, 2023

@zakkak thanks for your feedback, I will adjust the proposal according to some of the answers.

Thank you @vjovanov.

People often use the agent as the first step, but then they manually edit the config. In that case, I would prefer to have type as a starting point.

Sure, that makes sense as long as there is the option to exclude stuff from the type that was included via "type".

Same goes for "It seems strange that we will have two ways to achieve the same thing.". As long as we can achieve the same behavior it's fine to deprecate and eventually remove name.

@maxandersen
Copy link

Should we include the default constructors?

Is this question specific to type? If so, I would say yes.

From the conversation I understood it as if that happens then it would have negative impact on sizes and possible behaviors that's why it was suggested to leave out.

For me I see "type" as a shorthand, not for just including everything from a type but a decent good first choice which we want the agent as well as users to be able to use as much as possible by default - but then allow overrides.

People often use the agent as the first step, but then they manually edit the config. In that case, I would prefer to have type as a starting point.

Sure, that makes sense as long as there is the option to exclude stuff from the type that was included via "type".

Yeah, the more I think about it treating "type" as shorthand for setting the default of the details but be able to still use the detailed flags would make sense.

As long as we can achieve the same behavior it's fine to deprecate and eventually remove name.

Deprecate seems easy. Removal seems like very tricky. At what point would that be viable to consider?

@vjovanov
Copy link
Member Author

vjovanov commented Dec 7, 2023

For me I see "type" as a shorthand, not for just including everything from a type but a decent good first choice which we want the agent as well as users to be able to use as much as possible by default - but then allow overrides.

Great way to present the new feature. We all seem to agree that we need to keep the previous functionality.

Deprecate seems easy. Removal seems like very tricky. At what point would that be viable to consider?

We would have to analyze the dependencies on Maven and see how many people get impacted. I would not consider removal in the next 5 years.

@zakkak
Copy link
Collaborator

zakkak commented Jun 6, 2024

It looks like this was implemented in #8369. Are there any pending tasks on it?

@loicottet
Copy link
Member

#8369 and follow-up #8271 and #9043 are the implementation of this proposal. At this point the only change still needed is to backport the necessary parser changes to previous LTS versions so they are able to use the new format correctly.

@vjovanov vjovanov closed this as completed Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Released
Development

No branches or pull requests

5 participants