Speed up binding of @ConfigurationProperties with a large object graph #15760
Comments
spring-projects-issues
added
the
status: waiting-for-triage
|
Perhaps some cases we fixed in #13414 were missed. @dsyer is that |
|
There's a branch of initializr with the benchmark tests here: https://github.com/dsyer/initializr/tree/bench |
philwebb
added
type: task
|
Here's another (different) measurement: Where "small", "medium" and "large" mean the same as above. The benchmark this time is the time to start an empty Spring Boot app with just The key point is the difference - "medium" is 60ms "slower" than small, "large" is 1600ms "slower" than "medium". The difference between "large" and "medium" is a factor of roughly 10 in number of properties bound, but it takes more than a factor of 10 longer. However it scales, it's unpleasant. |
|
I wonder if the |
|
That's not the problem. The |
|
Using
The reduction in allocation also results in a reduction in GC activity with an increases in bindings per GC from 10 to 11.. Before: After: There also appears to be some improvement in overall performance. Note that the timings above appear relatively fast due to the JIT. I'm not yet convinced that the changes are generally suitable. The two changes may be quicker for the large YAML file but slower for other files with different property structure and property name lengths. |
|
There's a bug in I think it should be using the size instead: if (this.start.length == this.size) {With just this change in place, we see a drop in allocated bytes to roughly 12300000: |
|
Every little helps I guess. I tried those changes in the benchmarks and didn't see a huge improvement: |
|
We can buy ourselves another ~170000 bytes by combining the start and end arrays into a single |
|
I've started collecting some commits on this branch that improve things a little and, I believe, are not optimising specifically for Initializr's use of YAML. |
|
Some new numbers based on the current head of this branch: |
|
And some numbers from the Launcher Benchmark: Before: After: There's no discernible difference. Given that the benchmark measures a single cold-start binding, the changes are focussed on reducing allocations and, therefore, GC pressure, and that the properties can be bound 10 times before the GC runs, this isn't entirely surprising. The changes provided thus far don't appear to do any harm, and will hopefully provide more benefit with small heap sizes, but if we want to make this faster in the cold-start case, I think we'll need to look for changes that aren't specifically focussed on reducing GC pressure as I don't think GC is a major contributing factor to the performance. |
dsyer
changed the title
|
Here's some numbers gathered with a new harness comparing the current head of master with the current head of this branch: Large YAML file
Small YAML file
|
|
And here's a comparison of the changes proposed in #15782 (baseline) and the changes proposed in #15782 combined with those proposed here (new) for the large YAML file:
|
|
Pretty impressive results when we combine your and my work compared to the initial timings, @wilkinsona 👍 |
Previously, ElementsParser would expand its internal storage when the size of the storage was <= the end index of the element being parsed, irrespective of how many elements had been stored. This led to expansion of the storage, even for a source that contains a single element, if the end of the element was at an index greater than the size of the storage. This commit updates ElementsParser to resize its storage when the size (the number of elements that have been stored) is equal to the size of the storage. See gh-15760
Previously, the ElementsParser would be created using its default capacity of 6 even when parsing a String that is expected to produce a single element. This commit updates ConfigurationPropertyName to create an ElementsParser with a capacity of 1 when parsing a String that should contain only a single element. See gh-15760
Previously, when ConfigurationPropertyName was building the String returned from toString() it would use a StringBuilder with the default initial capacity of 16. For properties with several elements this was likely to be too small resulting in the builder's buffer being resized. This commit sizes the StringBuilder as a multiple of the number of elements in the name, attempting to strike a balance between allocating a StringBuilder with an initial capacity that's too large and wastes memory and an initial capacity that's too small and requires resizing. See gh-15760
|
Yeah, I'm really pleased with the improvements. Thanks for your help here, @dreis2211. It's greatly appreciated. A final set of numbers comparing 2.1.2.RELEASE and 2.2.0 snapshots with all the changes in place (
That's a 39.6% reduction in the mean startup time of an app that just binds the large YAML file to |
wilkinsona
changed the title
wilkinsona
added
type: enhancement
Uh oh!
There was an error while loading. Please reload this page.
@snicoll and I were looking at startup times in Initializr - there's a 2 second delay while the
InitializrPropertiesis being bound. Here are some benchmarks:UPDATE: I edited the table above to make the "size" metric actually match the size
of the properties bound (I don't know where JMH gets its counter metrics from sometimes).
The "large" sample above is using the same YAML as production app, and
0.641 ops/sec is getting on for 2 seconds per op (1.5 anyway). So it
sucks.
Looking in Flight Controller I'm seeing GC pressure from
ConfigurationPropertyName.buildString().List of property names passing through
ConfigurationPropertyName.buildString()on application start up: prop.txt. We can see that sometimes the method is called multiple times for the same property name (but only for indexed properties), and also that there is a lot of duplication in the indexed properties. Maybe that can be optimized?The text was updated successfully, but these errors were encountered: