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 object creation in RequestMappingHandlerMapping#handleNoMatch #29634

Closed

Conversation

koo-taejin
Copy link
Contributor

@koo-taejin koo-taejin commented Dec 4, 2022

related issue: #29611

RequestMappingInfoHandlerMapping handleNoMatch() method has the following format.
https://github.com/spring-projects/spring-framework/blob/main/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java#L246
https://github.com/spring-projects/spring-framework/blob/main/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java#L169

@Override
protected HandlerMethod handleNoMatch(Set<RequestMappingInfo> infos, ...) throws Exception {
	PartialMatchHelper helper = new PartialMatchHelper(infos, exchange);
	if (helper.isEmpty()) {
		return null;
	}
        ...
}

And this method may allow an empty value in Set<RequestMappingInfo> infos parameter.
(Unfortunately, it happens a lot in the service I am running.)

The constructor code of org.springframework.web.reactive.result.method.RequestMappingInfoHandlerMapping.PartialMatchHelper is shown below.
https://github.com/spring-projects/spring-framework/blob/main/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java#L227

public PartialMatchHelper(Set<RequestMappingInfo> infos, ServerWebExchange exchange) {
	this.partialMatches.addAll(infos.stream().
			filter(info -> info.getPatternsCondition().getMatchingCondition(exchange) != null).
			map(info -> new PartialMatch(info, exchange)).
			collect(Collectors.toList()));
}

This code executes follwoing method even if there is no value in infos
java.util.Collection#stream
java.util.stream.Collectors#toList
java.util.stream.ReferencePipeline#collect(java.util.stream.Collector<? super P_OUT,A,R>)
Unnecessary resources are used via these methods.(create objects and execute operations, so )

Even though parameters have values, it seems better not to use Stream.

It can save resources by changing the constructor like mvc's RequestMappingInfoHandlerMapping.
https://github.com/spring-projects/spring-framework/blob/main/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java#L300

public PartialMatchHelper(Set<RequestMappingInfo> infos, HttpServletRequest request) {
			for (RequestMappingInfo info : infos) {
				if (info.getActivePatternsCondition().getMatchingCondition(request) != null) {
					this.partialMatches.add(new PartialMatch(info, request));
				}
			}
		}

Below is a test created similar to the code above.

  1. style of webflux
  2. style of mvc
  3. style of reuse object
  • test code
public class PartialMatchHelperTest {

    @Test
    public void partialMatchHelperTest() {
        int count = 1000 * 1000 * 100;

        ThreadMXBean mxBean = ManagementFactory.getThreadMXBean();

        // 1)
        long startTime = System.currentTimeMillis();
        final long startCpuTime = mxBean.getCurrentThreadCpuTime();
        for (int i = 0; i < count; i++) {
            new PartialMatchHelper(Collections.emptySet());
        }
        System.out.println(System.currentTimeMillis() - startTime);
        System.out.println(mxBean.getCurrentThreadCpuTime() - startCpuTime);

        // 2)
        long startTime2 = System.currentTimeMillis();
        final long startCpuTime2 = mxBean.getCurrentThreadCpuTime();
        for (int i = 0; i < count; i++) {
            new PartialMatchHelper2(Collections.emptySet());
        }
        System.out.println(System.currentTimeMillis() - startTime2);
        System.out.println(mxBean.getCurrentThreadCpuTime() - startCpuTime2);

        // 3)
        long startTime3 = System.currentTimeMillis();
        final long startCpuTime3 = mxBean.getCurrentThreadCpuTime();
        for (int i = 0; i < count; i++) {
            PartialMatchHelper3.of(Collections.emptySet());
        }
        System.out.println(System.currentTimeMillis() - startTime3);
        System.out.println(mxBean.getCurrentThreadCpuTime() - startCpuTime3);
    }

    private static class PartialMatchHelper {
        private final List<PartialMatch> partialMatches = new ArrayList<>();

        public PartialMatchHelper(Set<RequestMappingInfo> infos) {
            partialMatches.addAll(infos.stream().
                    filter(info -> info.getPatternsCondition().getMatchingCondition(null) != null).
                    map(info -> new PartialMatch(info)).
                    collect(Collectors.toList()));
        }

        public boolean isEmpty() {
            return partialMatches.isEmpty();
        }
    }

    private class PartialMatchHelper2 {
        private final List<PartialMatch> partialMatches = new ArrayList<>();


        public PartialMatchHelper2(Set<RequestMappingInfo> infos) {
            for (RequestMappingInfo info : infos) {
                if (info.getMatchingCondition(null) != null) {
                    this.partialMatches.add(new PartialMatch(info));
                }
            }
        }

        public boolean isEmpty() {
            return partialMatches.isEmpty();
        }
    }

    private static class PartialMatchHelper3 {

        static PartialMatchHelper3 DISALBELD = new PartialMatchHelper3(Collections.emptySet());

        private final List<PartialMatch> partialMatches = new ArrayList<>();


        public static PartialMatchHelper3 of(Set<RequestMappingInfo> infos) {
            if (infos.isEmpty()) {
                return DISALBELD;
            } else {
                return new PartialMatchHelper3(infos);
            }
        }


        public PartialMatchHelper3(Set<RequestMappingInfo> infos) {
            for (RequestMappingInfo info : infos) {
                if (info.getMatchingCondition(null) != null) {
                    this.partialMatches.add(new PartialMatch(info));
                }
            }
        }

        public boolean isEmpty() {
            return partialMatches.isEmpty();
        }
    }

    private static class PartialMatch {

        private final RequestMappingInfo info;

        public PartialMatch(RequestMappingInfo info) {
            this.info = info;
        }


        public RequestMappingInfo getInfo() {
            return this.info;
        }

        @Override
        public String toString() {
            return this.info.toString();
        }
    }
}

Result (from Async Profiler)

    1) samples : CPU   393, Memory allocations      13,474,672
    2) samples : CPU   580, Memory allocations   4,804,133,448
    3) samples : CPU  6,547, Memory allocations 37,581,912,072

In my opinion, the following order seems like a good way.
(Suggested solution (3 + 2) -> 3 -> 2 -> 1)

I will make PR according to your opinion.

Thanks :)

@koo-taejin
Copy link
Contributor Author

I have worked for Only the initialization part mentioned in the issue.
The performance of Stream itself is not good, so it would be better if other parts were improved.
http://www.angelikalanger.com/Conferences/Videos/Conference-Video-GeeCon-2015-Performance-Model-of-Streams-in-Java-8-Angelika-Langer.html

If you want to change the other parts to the collector's loop as before.
Please let me know, then I am going to work for it.

Thanks :)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 4, 2022
@rstoyanchev rstoyanchev self-assigned this Dec 5, 2022
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 5, 2022
@rstoyanchev rstoyanchev added this to the 6.0.3 milestone Dec 5, 2022
@rstoyanchev rstoyanchev changed the title [#29611] Optimized resource usage when invoke handleNoMatch Optimize resource usage of RequestMappingHandlerMapping#handleNoMatch Dec 5, 2022
@rstoyanchev rstoyanchev changed the title Optimize resource usage of RequestMappingHandlerMapping#handleNoMatch Optimize object creation in RequestMappingHandlerMapping#handleNoMatch Dec 9, 2022
@rstoyanchev rstoyanchev added the status: backported An issue that has been backported to maintenance branches label Dec 9, 2022
@rstoyanchev
Copy link
Contributor

Backported to 5.3.25 with #29667.

rstoyanchev pushed a commit that referenced this pull request Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants