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

Generate mapping names #1146

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Generate mapping names #1146

merged 1 commit into from
Apr 22, 2024

Conversation

radcortez
Copy link
Member

No description provided.

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 16, 2024

Does it not seem like the complexity is getting a little out of control here? I don't think this code is understandable at all without careful analysis - and that's a big problem IMO.

@radcortez
Copy link
Member Author

Maybe. Most of the work so far was moving things around and trying to simplify a few things (which I guess I failed :)). There is even some more complexity coming. The goal was to make it more build-time friendly.

If you remember, we relied on introspecting the mapping classes to be able to construct the instances. This started to have a negative impact on Quarkus, with more extensions moving to use mappings, so I've refactored a lot of things to get rid of any class introspection requirements during runtime. At the moment, the only information we need at runtime is the path patterns and to which classes they belong. With this information, when we build the instance, we can query if lazy elements are required or not (cases of Map with a Group or an Optional with a Group), by matching the available properties in the Config system to these path patterns.

At the moment, there are some method calls required during Quarkus build to construct the mapping names metadata and feed it to the builder, but my idea was always to make it self contained, meaning that the mapping would generate it itself in its implementation and feed it to the config. The work on this PR was the preparation for that, so the complex part missing is the bytecode generation to provide that information.

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 16, 2024

How much more ad-hoc work can we afford to spend time on before we can justify looking at architectural solutions instead? The way things are now, every complex change increases the complexity of the next change, and so on. It's going to become completely unmanageable soon (well, it is already, to be honest: if you were abducted by a flying saucer tomorrow, we'd be in trouble). We need an architectural solution.

@radcortez
Copy link
Member Author

I agree. At the same time, we also need to keep the current implementation on par with Quarkus requirements.

I know this is a big promise, but I believe no more big changes will be required after this. The current implementation works as expected, and we were able to greatly decrease resource usage.

We generate the required metadata during Quarkus build, which does the job. However, there are cases where users may need to create their own Config instance outside our own, which falls back to runtime introspection. For that reason, we still need to register mapping methods for reflection, and with the usage increase, it is also increasing the native image size for no good reason: quarkusio/quarkus#39748

The original idea was to generate metadata directly in SR Config, but I wanted to try out and measure the new implementation, so it was just easier to do it in Quarkus.

@radcortez radcortez force-pushed the mapping-names branch 5 times, most recently from 1ee7d06 to 362e8ce Compare April 20, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants