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

Improved resource usage when invoke RequestMappingInfoHandlerMapping.handleNoMatch #29611

Closed
koo-taejin opened this issue Nov 30, 2022 · 2 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another

Comments

@koo-taejin
Copy link
Contributor

koo-taejin commented Nov 30, 2022

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 :)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 30, 2022
@rstoyanchev
Copy link
Contributor

Thanks for bringing this up. Please, go ahead with submitting a PR. It seems reasonable to make such an optimization.

@rstoyanchev rstoyanchev added this to the Triage Queue milestone Dec 2, 2022
@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Dec 2, 2022
@rstoyanchev rstoyanchev self-assigned this Dec 2, 2022
koo-taejin added a commit to koo-taejin/spring-framework that referenced this issue Dec 4, 2022
koo-taejin added a commit to koo-taejin/spring-framework that referenced this issue Dec 5, 2022
koo-taejin added a commit to koo-taejin/spring-framework that referenced this issue Dec 5, 2022
@rstoyanchev
Copy link
Contributor

Superseded by #29634.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2022
@rstoyanchev rstoyanchev added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 5, 2022
@rstoyanchev rstoyanchev removed this from the Triage Queue milestone Dec 5, 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: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

3 participants