Various suggestions

[gsmet]

@radcortez I pushed several suggestions here. Some are easy wins that we could apply without too much discussion, some are maybe more controversial.

I would very much like to discuss Provide some shortcuts for simple config properties as this commit clearly removes some work (and I checked the generated bytecode and it looks like it's actually less work) but it seems to make things a bit slower and I wasn't able to pinpoint what was going on.

Interested in your feedback :).

[radcortez]

Thanks!

Let me have a better look at the bytecode pieces. For the remaining changes, I think we should open separate PR's.

[radcortez]

I've made a similar proposal in #1249, but we ended up not moving forward with it.

[gsmet]

Yeah I can understand, it's quite messy. Let's not consider it then.

[gsmet]

Let me have a better look at the bytecode pieces.

Yeah, very interested in your feedback. I banged my head on it for a while. It should be faster but from my tests, it's consistently a tiny bit slower.

For the remaining changes, I think we should open separate PR's.

Done for the ones that had a +1.

[radcortez]

Great! Thanks!

[radcortez]

Yeah, very interested in your feedback. I banged my head on it for a while. It should be faster but from my tests, it's consistently a tiny bit slower.

I had a look and even expanded the idea further, by applying it to all leaf elements:
#1286

There is a marginal improvement compared to the same kind of work, which almost feels not worth it. I measured 6882 k samples vs 6861 k, so nothing major.

Now, if you were comparing old root vs new mapping for Vert.x config, yes, you are always going to notice a tiny decrease because we do a few extra things:

• 2 classes to load instead of one (interface + implementation vs just the class)
• We keep track of used properties
• We load defaults directly from the implementation
• We convert the property names on the fly for the expected naming strategy

It's a bit tricky to get rid of these:

• The used properties are to report the list of unknowns. We end up ignoring the quarkus properties because Quarkus has its own way of checking for unknowns (due to the config phases). This is still useful for application mappings.
• We store the defaults in the implementation and load them with the mapping, so it doesn't depend on any external processor to provide that information. One issue with the old roots is that they couldn't be used outside Quarkus. We could add something to skip that from the Quarkus side and provide a source from build time. Traditional runtime would still require loading them from the implementation.
• The naming strategy is mostly because you override the naming on any part of the tree, and because you can reuse mapping definitions, one declaration on the parent can use one naming strategy and the same definition on another declaration another one, so we can't generate the final name of the property to look for directly in the implementation, since it depends on the declaration.

We can definitely look into improving some things, but it does feel that the gain would be marginal. For instance, the quarkus.uuid change has a more significant impact than all of these combined (plus a few other things I did). I think we should look into other areas before pursuing these extra things.

[gsmet]

There is a marginal improvement compared to the same kind of work, which almost feels not worth it. I measured 6882 k samples vs 6861 k, so nothing major.

My reasoning for trying to improve this was that once all the extensions are moved to @ConfigMapping, we will have a lot more of them and reducing the price you pay for each might help.

Especially since this price is paid just by having more config properties around in your app, not by actually using them.

The naming strategy is mostly because you override the naming on any part of the tree, and because you can reuse mapping definitions, one declaration on the parent can use one naming strategy and the same definition on another declaration another one, so we can't generate the final name of the property to look for directly in the implementation, since it depends on the declaration.

I can see why it could be useful in some cases but I wonder if we could mark trees specifically to disable this feature? For anything that is in the core Quarkus extensions, I don't think we need this flexibility?

Note that it might be a stupid idea but, if this cost is important, we could add additional constraints to the Quarkus config - especially since I don't think it makes sense to reuse the Quarkus config in other trees - and if you do, you would have to follow our naming convention.

[radcortez]

My reasoning for trying to improve this was that once all the extensions are moved to @ConfigMapping, we will have a lot more of them and reducing the price you pay for each might help.

Especially since this price is paid just by having more config properties around in your app, not by actually using them.

Absolutely, you are correct. That is why I was suggesting that we look into things from another angle. While every bit counts, we also know these will be marginal. If you consider an application with vertx, arc and rest, it has more than 600 configurations. Each configuration has to be queried in each source until found, which in most cases is the default, or not found in the case of optionals. Lookups are actually double because we also look for the profiled configuration. Rough numbers: 4 sources * 600 configurations * 2 profiles. We could probably improve the situation if we could think of a way to considerably reduce the number of lookups. I have a couple of ideas, but it would be great to have other people think about this.

I can see why it could be useful in some cases but I wonder if we could mark trees specifically to disable this feature? For anything that is in the core Quarkus extensions, I don't think we need this flexibility?

Note that it might be a stupid idea but, if this cost is important, we could add additional constraints to the Quarkus config - especially since I don't think it makes sense to reuse the Quarkus config in other trees - and if you do, you would have to follow our naming convention.

I didn't measure that cost specifically, and yes, we don't need this in our Quarkus config. It is not a stupid idea, is just a matter of measuring the effort and benefit. My gut feeling is that the benefit would be marginal, but let me measure it first and see what we can gain. Again, I'm not saying we shouldn't do it, but I think that we can probably have better improvements in other areas.