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

Suboptimal efficiency of request parameters parsing #2425

Open
Sanne opened this issue May 18, 2023 · 1 comment
Open

Suboptimal efficiency of request parameters parsing #2425

Sanne opened this issue May 18, 2023 · 1 comment
Labels

Comments

@Sanne
Copy link
Contributor

Sanne commented May 18, 2023

Version

4.4.2

Context

I've been running Techempower benchmarks with the type pollution agent enabled, to identify subtle scalability issues in Quarkus and Vert.x.

Do you have a reproducer?

I reproduced this issue by running the Techempower suite, but I'm pretty sure any vert.x-web application running under load with the agent would trigger this. It might be important to not have a "too simple" application as it also needs some code to attempt using ArrayList in a different context.

Type pollution report

7:	java.util.ArrayList
Count:	30848
Types:
	java.util.List
	java.lang.Iterable
	java.util.Collection
Traces:
	io.vertx.ext.web.impl.RoutingContextImpl.getQueryParams(RoutingContextImpl.java:467)
		class: java.lang.Iterable
		count: 15371
	org.postgresql.jdbc.PgResultSet.initRowBuffer(PgResultSet.java:3398)
		class: java.util.List
		count: 13064
	org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.list(JdbcSelectExecutorStandardImpl.java:93)
		class: java.util.List
		count: 2397

Patch proposal / brainstorming

The method RoutingContexImpl#getQueryParams(Charset) includes the following two lines:

 Map<String, List<String>> decodedParams = new QueryStringDecoder(request.uri()).parameters();
 for (Map.Entry<String, List<String>> entry : decodedParams.entrySet()) {

I was going to suggest a workaround for the type pollution only, but then I realized: the QueryStringDecoder is allocated and then thrown away, to invoke only that single method which allocates a Map, fills it in, and then this Map is discarded as its content gets copied into the MultiMap.

That seems like a lot of allocations when what we need is only the output MultiMap; perhaps the whole logic should be refactored so to populate the MultiMap right away while parsing?

The type-pollution issue is likely to vanish as side-effect of a cleaner rewrite; a bit annoying that QueryStringDecoder is in Netty and it might need to be rewritten.

@Sanne Sanne added the bug label May 18, 2023
@vietj
Copy link
Contributor

vietj commented May 22, 2023

QueryStringDecoder is from Netty and it decodes to a Map<String, List<String>> and the default MultiMap implementation instead has an internal structure that handle things differently. So we would have to get a dedicated MultiMap implementation that wraps Map<String, List<String>>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants