Skip to content

Improve binding performance when using a large number of property sources #20625

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.

Sign up for GitHub

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

Jump to bottom
Closed
smaldini opened this issue Mar 23, 2020 · 8 comments
Closed

Improve binding performance when using a large number of property sources #20625

smaldini opened this issue Mar 23, 2020 · 8 comments
Assignees
@philwebb
Labels
type: enhancement A general enhancement
Milestone
2.3.0

Comments

@smaldini
Copy link

smaldini commented Mar 23, 2020 •
edited
Loading

Hey team !

We have an interesting issue where our propertySource use can be CPU taxing on our systems due to 1/ many propertySources setup (sometimes 80+), 2/ many properties setup over those propertySources (sometimes 1000+).

It seems like the reason is the support for relaxed binding and use of ConfigurationPropertySourcesPropertySource in the first position of the list of propertySources although it can be reproduced when "forced" in last position as well. The property resolution is doing a breadth first search of all possible bindings in this layer then it's doing an exact match again a second time. Our flamegraph have revealed a lot of String manipulations happening in hot paths where we use environment.getProperty.

In our use cases we use environment.getProperty sometimes more than once per request since some properties could have changed. One use case for instance is a dynamic timeout. I imagine any use case with a MutablePropertySource used in a request or messaging path would reproduce that problem.

We are envisioning caching as a possible solution:

  • a new first position property source acting as a cache
  • caching hit and misses and if a mutable source is present, syncing any refresh with this cache
  • optionally providing metrics to that cache to analyse property resolutions patterns

The cache could be lazily built or partially (maybe fully?) hydrated on startup if desired although this should be an opt-in given the impact it could have on startup time.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 23, 2020
@mbhave mbhave added the for: team-attention An issue we'd like other members of the team to review label Mar 25, 2020
@philwebb
Copy link
Member

@smaldini Are you able to put together something that replicates the issue? It would be really helpful if we could reproduce this locally so that we can assess the impact of any cache changes we make.

@philwebb philwebb added status: waiting-for-feedback We need additional information before we can continue and removed for: team-attention An issue we'd like other members of the team to review labels Mar 26, 2020
@philwebb
Copy link
Member

Also, is it possible to share the flame graph.

@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Apr 2, 2020
@spring-projects-issues
Copy link
Collaborator

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Apr 9, 2020
@philwebb philwebb reopened this Apr 9, 2020
@philwebb philwebb added status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Apr 9, 2020
@mbhave
Copy link
Contributor

mbhave commented Apr 13, 2020

One thing I realized is that we might be doing a double lookup for missing properties. I don't think this is related to the slowness but I was wondering if there is something we can do avoid that. The ConfigurationPropertySourcesPropertySource wraps all other property sources in the environment. Since it's just another PropertySource, if the property is not found in that source, environment.getProperty will continue looking for it in the other property sources. These property sources have already been searched for because they are wrapped by ConfigurationPropertySourcesPropertySource.

@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Apr 16, 2020
@smaldini
Copy link
Author

Sorry i didn't look at those notifications - apologies @philwebb @mbhave ! I can't easily provide the flamegraph that showed this but I can try to do a repro case, also the test would be very simple, just a JMH over environment .getProperty with a missed property, vs a property in various locations (first and last).

@mbhave suggestion looks good, if we miss in the ConfigurationPropertySourcesPropertySource we shouldn't go further.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Apr 17, 2020
@philwebb
Copy link
Member

philwebb commented May 1, 2020

See also #17400

@philwebb philwebb changed the title Add a cacheable PropertySource layer Performance degrates when using a large number of propery sources May 4, 2020
@philwebb philwebb changed the title Performance degrates when using a large number of propery sources Performance degrades when using a large number of propery sources May 4, 2020
@philwebb philwebb self-assigned this May 4, 2020
@philwebb philwebb added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels May 7, 2020
@philwebb philwebb added this to the 2.3.0 milestone May 7, 2020
philwebb added a commit that referenced this issue May 7, 2020
@philwebb
Test large property source performance
5309912 
Add a test to ensure that a large number of property sources that each
contain many items can perform well.

See gh-20625
philwebb added a commit that referenced this issue May 7, 2020
@philwebb
Add hashcode support to ConfigurationPropertyName
85e9a73 
Provide a hashcode implementation for `ConfigurationPropertyName` so
that instances can be stored in Map without them all ending up in the
same bucket.

See gh-20625
sly1615096842 added a commit to sly1615096842/spring-boot that referenced this issue May 8, 2020
@sly1615096842
Add ConfigurationPropertyCaching support
60f45df 
Add a `ConfigurationPropertyCaching` utility interface that can be
used to control the property source caching.

Prior to this commit, a `ConfigurationPropertySource` that was backed
by a mutable `EnumerablePropertySource` would need to call the
`getPropertyNames()` method each time a property was accessed. Since
this this operation can be expensive, we now provide a way to cache
the results for a specific length of time.

This commit also improves the performance of immutable property sources
by limiting the number of candidates that need to be searched.
Previously, all mapped names would be enumerated. Now, mappings are
grouped by `ConfigurationPropertyName`. This is especially helpful when
the `ConfigurationPropertyName` isn't mapped at all since the hash based
map lookup will be very fast and the resulting mappings will be empty.

Closes spring-projectsgh-20625
@dreis2211 dreis2211 mentioned this issue May 12, 2020
Closed
@wilkinsona wilkinsona changed the title Performance degrades when using a large number of propery sources Improve binding performance when using a large number of property sources May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@philwebb philwebb

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.3.0
Development

No branches or pull requests
4 participants
@smaldini @philwebb @mbhave @spring-projects-issues