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

Optimize DefaultLifecycleProcessor::startBeans #25506

Closed
wants to merge 3 commits into from

Conversation

chenqimiao
Copy link
Contributor

@chenqimiao chenqimiao commented Aug 2, 2020

Use lambda for minor refactoring

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 2, 2020
phases.put(phase, group);
}
LifecycleGroup group = phases.computeIfAbsent(phase,
p -> new LifecycleGroup(phase, this.timeoutPerShutdownPhase, lifecycleBeans, autoStartupOnly));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation of computeIfAbsent says:

return the current (existing or computed) value...

so you could do:

phases.computeIfAbsent(
						phase,
						p -> new LifecycleGroup(phase, timeoutPerShutdownPhase, lifecycleBeans, autoStartupOnly)
				).add(beanName, bean);

But looking at the implementation, phases is really just a sorted by Keys, which mean it's really supposed to be a TreeMap? So that code could be written as :

	private void startBeans(boolean autoStartupOnly) {
		Map<String, Lifecycle> lifecycleBeans = getLifecycleBeans();
		Map<Integer, LifecycleGroup> phases = new TreeMap<>();

		lifecycleBeans.forEach((beanName, bean) -> {
			if (!autoStartupOnly || (bean instanceof SmartLifecycle && ((SmartLifecycle) bean).isAutoStartup())) {

				int phase = getPhase(bean);
				phases.computeIfAbsent(
						phase,
						p -> new LifecycleGroup(phase, timeoutPerShutdownPhase, lifecycleBeans, autoStartupOnly)
				).add(beanName, bean);
			}
		});

		phases.forEach((key, value) -> value.start());
	}

The use of forEach is safe since it is documented as :

Unless otherwise specified by the implementing class, actions are performed in the order of entry set iteration

and TreeMap::entrySet is documented as :

The set's iterator returns the entries in ascending key order...

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you said makes sense, but I added a judgment to reduce the execution of the iterator automatic code.
Thanks !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, but can you explain "but I added a judgment to reduce the execution of the iterator automatic code" - what this means? I don't get it. thank you

@sbrannen sbrannen self-assigned this Aug 10, 2020
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 10, 2020
@sbrannen sbrannen changed the title Minor refactor Optimize DefaultLifecycleProcessor::startBeans Aug 10, 2020
@sbrannen sbrannen added this to the 5.3 M2 milestone Aug 10, 2020
@sbrannen sbrannen closed this in af8ab2e Aug 10, 2020
sbrannen added a commit that referenced this pull request Aug 10, 2020
@sbrannen
Copy link
Member

This has been merged into master and slightly revised in 1f35cc5.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants