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

[GR-50385] Include all reflection queries for registered classes #9043

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Jun 4, 2024

This PR uses the work done in #8369 and #8271 to automatically register all elements of a class registered for reflection with the newly-introduced "type" keyword. The methods, constructors and fields included only have the metadata required to create the corresponding Method, Field or Constructor object without invocation or access capabilities. This change has therefore no influence on reachability.
The image size impact of this change is documented in #7753. Including all the metadata incurs a less than 2% image size increase in all evaluated workloads, with an average around 1%.
This PR includes the following changes:

  • Enable the registration of queried-only fields in ConfigurationType. This is not user-facing, but is necessary to be able to register all fields in a class for reflection without affecting reachability.
  • Modifying the ReflectionConfigurationParser to register the metadata when "type" is used. This part required some modification to the previous work done when introducing "type": the whole processing is now contained in the parser, and the ConfigurationType is now unaware of whether it was created from a "type" or "name" entry.
  • FrequencyEncoder is modified to enable a check when adding a new element. This is used to make debugging easier in cases where a class that was omitted from the static analysis is added to the encoder.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 4, 2024
@graalvmbot graalvmbot closed this Jun 5, 2024
@graalvmbot graalvmbot deleted the lottet/GR-50385-all-included branch June 5, 2024 11:23
@graalvmbot graalvmbot merged commit 9f1232f into master Jun 5, 2024
13 checks passed
@zakkak
Copy link
Collaborator

zakkak commented Jun 6, 2024

Hi @loicottet, could you please add a description to the PR?

If I am not mistaken this is related to #7753. It also seems to bypass the proposal in #7962, is that correct?

The reason this was opened and merged that fast I assume was to get it in before the Rampdown Phase One (aka feature freeze), but that leaves us with no time to properly test it and seems risky for a change in the defaults (that we don't seem to have a way to revert through an option). Are there any plans on further patches on top of it that would allow us to avoid this new behavior if needed?

cc @maxandersen

@zakkak
Copy link
Collaborator

zakkak commented Jun 6, 2024

It also seems to bypass the proposal in #7962, is that correct?

Correcting myself. The proposal in #7962 (to add a new way to register classes that will trigger automatic registration of all metadata) seems to have been implemented in #8369 and this PR seems like a follow up.

@loicottet
Copy link
Member

The description of the PR should have been copied when mirroring, sorry about that. This PR's point is to "flip the switch" on #8369, so that using "type" actually registers all reflective queries having no impact on reachability.

@zakkak
Copy link
Collaborator

zakkak commented Jun 11, 2024

The description of the PR should have been copied when mirroring, sorry about that.

Thanks for the prompt reply @loicottet. It looks like this is broken in general (I see descriptions missing in most if not all recent mirrored PRs), could you please have someone look into it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants