Skip to content

Avoid parsing and decoding the query when extracting the request path prefix - #912

Closed
parker89 wants to merge 1 commit into
apache:masterfrom
parker89:pw/path-prefix-extraction
Closed

parker89 wants to merge 1 commit into
apache:masterfrom
parker89:pw/path-prefix-extraction

Conversation

@parker89

@parker89 parker89 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

RequestSupport#extractPathPrefix runs on every request through ProtocolExec, AsyncProtocolExec and RequestAuthCache, whether or not authentication is configured. It builds a URIBuilder from the whole request target, so every query parameter gets percent-decoded only to be thrown away by clearParameters(). Then the path is parsed by java.net.URI, decoded segment by segment and re-encoded, even when there is nothing to normalize.

In our production profiles this method showed up in almost every service using the classic client, at a median of about 0.6% CPU and 1.2% of allocation per service and up to about 6.6% CPU / 11.6% allocation for services that send long query strings.

This change:

  • stops at the first ? or #, so the query and fragment are never parsed
  • returns the prefix directly when the path is already in normalized form (leading /, unreserved characters only, no empty or dot segments), which is the common case
  • otherwise runs the existing URIBuilder normalization on the path alone, falling back to the full request target if the path by itself isn't a valid URI reference (e.g. //?x)

The result is the same as before for every request target that is a valid URI, including %2F, non-normalized percent encoding, dot segments, empty segments and ; parameters. The existing assertions are unchanged. I added a randomized test that compares the new code against the previous implementation over a mix of those cases, plus explicit cases for queries with encoded characters, fragments, trailing slashes and encoded slashes.

One intentional difference: when the path is valid but the query or fragment has characters java.net.URI rejects (e.g. /a/b?q=x y), the old code returned the whole request target, query included, as the prefix. Now it returns /a/.

JMH, JDK 21, average time and gc.alloc.rate.norm:

Request target before ns/op after ns/op before B/op after B/op
no query 651 41 (-94%) 1,664 56 (-97%)
3 params 2,093 97 (-95%) 4,824 96 (-98%)
6 params, percent-encoded values 2,888 42 (-99%) 6,340 56 (-99%)
10 params 4,646 115 (-98%) 10,232 104 (-99%)
4 params, %2F in the path (still uses URIBuilder) 2,882 1,462 (-49%) 5,872 2,848 (-51%)

Happy to share the benchmark if useful, I didn't want to add a JMH module just for this.

The commit also applies cleanly to 5.6.x and passes there against httpcore 5.4.4, so it would be great to get it backported for the next 5.6.x release.

@ok2c

ok2c commented Oct 30, 2026

Copy link
Copy Markdown
Member

@parker89 Thank you for bringing the problem to our attention and proposing a fix. This particular piece of functionality was introduced in order to tighten conformance to the HTTP spec but it is of little practical use, actually. I now regret having introduced it in the first place.

I would like to approach this problem from two angles: (1) see if this feature could be made context dependent (applicable to BASIC and DIGEST auth only) (2) optimization of the path extraction code as you suggested. I. however, would really like to avoid having to have another path normalization routine just for this specific case. I would like to see if there another way of solving the problem. Please bear with me.

@ok2c

ok2c commented Oct 31, 2026 •

Copy link
Copy Markdown
Member

@parker89 Here is what I propose as the first step that should be fully backward compatible while eliminating most of the unnecessary parsing and decoding: #915 This change-set could go into both master and 5.6.x lines

Next, we could think further improvements but in master only

@parker89

parker89 commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for picking this up so quickly, closing in favor of #915.

Two follow-ups, if useful:

  • RequestSupport#extractPathPrefix optimization #915 still takes the slow path for any . in the path, so dotted segments and file extensions (/packages/com.example.service/versions/1.2.3, /files/report.csv) only drop from ~2.1 µs / 5 KB to ~1.3-1.6 µs / 3.1-3.4 KB. Only flagging a . that starts a segment (ch == '.' && (previous == '/' || previous == 0)) keeps those on the fast path, and gives the same results as the merged code for 200k generated paths. Happy to send a small PR with tests. (Edit: went ahead and put it up here: Keep request paths with dots inside segments on the fast path when extracting the path prefix #927)
  • For 5.6.x, PathSegmentCodec isn't in httpcore 5.4.x, so a backport would need the split/decode inlined. I can do that PR too if you'd like it on 5.6.x.

@parker89 parker89 closed this Nov 3, 2026
@ok2c

ok2c commented Nov 3, 2026

Copy link
Copy Markdown
Member

For 5.6.x, PathSegmentCodec isn't in httpcore 5.4.x, so a backport would need the split/decode inlined. I can do that PR too if you'd like it on 5.6.x.

@parker89 I have already ported the change-set to 5.6.x. See aeceb99

@ok2c

ok2c commented Nov 4, 2026

Copy link
Copy Markdown
Member

@parker89 This change-set #929 is probably even more important as far as path prefix handling optimization is concerned. It ensures a path prefix gets extracted on demand only if there has been a auth challenge from the opposite endpoint. Please take a look.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants