vertx-web icon indicating copy to clipboard operation
vertx-web copied to clipboard

Suboptimal efficiency of request parameters parsing

Open Sanne opened this issue 2 years ago • 1 comments

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 avatar May 18 '23 11:05 Sanne

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

vietj avatar May 22 '23 07:05 vietj