spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.48k stars 40.53k forks source link

Attempt to improve performance with SpringIterableConfigurationPropertySource #28723

Open pintomau opened 2 years ago

pintomau commented 2 years ago

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

quaff commented 2 years ago

Why would you do that?

philwebb commented 2 years ago

@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.

pintomau commented 2 years ago

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.

AlcipPopa commented 2 years ago

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.

wilkinsona commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

rezekdan commented 2 years ago

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 commented 2 years ago

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

philwebb commented 2 years ago

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 https://github.com/spring-cloud/spring-cloud-config/issues/2032 to see if the Spring Cloud team can change things on their side.

philwebb commented 2 years ago

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

pintomau commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

@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?