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

Attempt to improve performance with SpringIterableConfigurationPropertySource #28723

Open
pintomau opened this issue Nov 17, 2021 · 14 comments
Open
Labels
theme: performance Issues related to general performance type: enhancement A general enhancement
Milestone

Comments

@pintomau
Copy link

Running a project with >10k external properties.

It seems that Spring is taking a long time with processing all property binding, and also contains a deep stack.

I'm not sure how to proceed further with this or optimize, or if this is simply that needs to be worked on from a performance point.

image

Flight recording attached.

flight-record.zip

Spring boot 2.3.12

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 17, 2021
@quaff
Copy link
Contributor

quaff commented Nov 18, 2021

Why would you do that?

@philwebb
Copy link
Member

@pintomau You might want to upgrade to Spring Boot 2.5 or 2.6 to see if #17400 makes any difference to your situation. I suspect it might not because we haven't encountered anywhere near that number of external properties in other projects. If it doesn't help, could you please provide a sample application that shows exactly what you're doing.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Nov 18, 2021
@pintomau
Copy link
Author

pintomau commented Nov 18, 2021

It's hard for us to update since we have some hard dependencies on spring cloud, that were removed or otherwise need to be adapted to.

Need some time to reproduce this otherwise.

Why would you do that?

Multi-tenant platform, dozens of tenants, hundreds of different configurations each.

Thanks.

@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 labels Nov 18, 2021
@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Nov 18, 2021
@AlcipPopa
Copy link

Running a project with >10k external properties.

Having so many external properties is bad design, my friend. You should avoid loading them at application startup. I would suggest doing it at runtime, at first feature execution; there's really something bad happening with your design there: it's like replacing database data with SpringBoot properties really.

@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 labels Nov 21, 2021
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Nov 22, 2021
@wilkinsona
Copy link
Member

wilkinsona commented Nov 22, 2021

Without more context, I don't think anyone's in a position to describe the design of @pintomau's app as bad. Until they express an interest in some advice on an alternative approach or provide some more details about the goals and constraints that led to the current design, let's not jump to making negative judgments please.

We said above that we're happy to take a look and see what we can do to speed things up once we have a sample that reproduces the problem. Speeding up the processing of 1000s of properties may well benefit apps with only 10s or 100s of properties as well.

@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 Nov 29, 2021
@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 Dec 6, 2021
@rezekdan
Copy link

i hacked down a small and simplistic sample to showcase the issue: https://github.com/rezekdan/boottimes

TLDR: 10k config bindings each binding 4 properties. binding 40k of properties for this simple application takes two minutes to boot on my machine. apart from binding this sample does nothing else, just using spring as it is intended by the documentation. i confirmed with @pintomau , that the profiler shows very similar results for the sample app as it does for the original application case.

@wilkinsona wilkinsona reopened this Jan 12, 2022
@wilkinsona
Copy link
Member

Thanks, @rezekdan. We'll take a look.

@wilkinsona wilkinsona added the status: waiting-for-triage An issue we've not yet triaged label Jan 12, 2022
@philwebb
Copy link
Member

This is due to our old friend SpringIterableConfigurationPropertySource having to create mapping each time the source is accessed. One thing that would speed things up is if the property source added by Spring Cloud were marked as immutable.

With the following hack applied to the sample:

@SpringBootApplication
@EnableConfigurationProperties({ BootTimesConfiguration.class })
public class BoottimesClientApplication {

	public static void main(String[] args) {
		SpringApplication application = new SpringApplication(BoottimesClientApplication.class);
		application.addListeners(new HackImmutable());
		application.run(args);
	}

	private static class HackImmutable implements ApplicationListener<ApplicationEnvironmentPreparedEvent> {

		@Override
		public void onApplicationEvent(ApplicationEnvironmentPreparedEvent event) {
			Field field = ReflectionUtils.findField(OriginTrackedMapPropertySource.class, "immutable");
			field.setAccessible(true);
			ConfigurableEnvironment environment = event.getEnvironment();
			for (PropertySource<?> source : environment.getPropertySources()) {
				if (source.getName().startsWith("configserver:")) {
					ReflectionUtils.setField(field, source, true);
				}
			}
		}

	}

}

The startup time drops from 38.455 seconds to 4.498 seconds on my local laptop.

I've raised spring-cloud/spring-cloud-config#2032 to see if the Spring Cloud team can change things on their side.

@philwebb
Copy link
Member

I'd like to leave this issue open to see if there's any other way we can improve things.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 15, 2022
@philwebb philwebb changed the title Slow project boot with considerable amount of properties Attempt to improve performance with SpringIterableConfigurationPropertySource Jan 15, 2022
@philwebb philwebb added this to the 2.x milestone Jan 15, 2022
@philwebb philwebb added the theme: performance Issues related to general performance label Jan 15, 2022
@pintomau
Copy link
Author

pintomau commented Jan 17, 2022

Hi @philwebb .

To expand on the issue, we tried it in our project, but we noticed some differences between the PoC @rezekdan developed and our own.

We're using redis as the database for properties. We noticed that the redis properties are not affected by the hack you provided (maybe we could use a different event?).

Regardless, we've shadowed OriginTrackedMapPropertySource#isImmutable to always return true and we still have another issue.

Spring cloud config client seems to wrap Redis' OriginTrackedMapPropertySource in BootstrapPropertySource. Then, of course, source instanceof OriginLookup in SpringIterableConfigurationPropertySource returns false, which means isImmutable won't be called.

We again resorted to shadowing SpringIterableConfigurationPropertySource and added some lines to take this wrapper into consideration:

EnumerablePropertySource<?> source = getPropertySource();
    if (source instanceof BootstrapPropertySource) {
      source = (EnumerablePropertySource<?>) ((BootstrapPropertySource) source).getDelegate();
    }
    if (source instanceof OriginLookup) {
      return ((OriginLookup<?>) source).isImmutable();
    }
    if (StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME.equals(source.getName())) {
      return source.getSource() == System.getenv();
    }
    return false;

As you mentioned, this substantially improves performance. I'm not sure about all, if any side effects there are.

Of course, we'd also like to not resort to shadowing. Maybe you have some idea?

Thanks!

@nithril
Copy link

nithril commented Apr 13, 2023

Same issue there with Spring Boot 2.7.

We are using ConfigurationProperties capabilities to load the application configurations that consists in around 30k lines of yaml. It is a list of search fields dynamically generated from an excel file.

searchFields:
  - searchNames:
      - foo
    elasticFields:
      - elasticName: foo
        converter: defaultSearchFieldConverter
        fieldType: KEYWORD

The approach benefits from the out of the box java bean validation (the fields are annotated with javax validation) and the native Spring converters. There converter is a spring bean.

The loading/binding takes approximately 30s.

Would be for sure faster by loading the yaml outside Spring capabilities but we would have to do the validation and implement some converters.

@wilkinsona
Copy link
Member

@nithril 30k lines is quite extreme and does make me wonder if you'd be better using a different approach. That said, we do still want to try to improve things here. To that end, can you please share an example, with synthetic data if needed, that reproduces the slow binding performance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: performance Issues related to general performance type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

9 participants