vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 536 forks source link

Suboptimal efficiency of request parameters parsing #2425

Open Sanne opened 1 year ago

Sanne commented 1 year ago

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.

vietj commented 1 year ago

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