pjfanning opened a new pull request, #1229:
URL: https://github.com/apache/pekko-http/pull/1229
### Motivation
`asciiString` (`impl/engine/parsing/package.scala`) built its `String` one
character at a time:
```scala
@tailrec def build(ix: Int = start, sb: JStringBuilder = new
JStringBuilder(end - start)): String =
if (ix == end) sb.toString else build(ix + 1, sb.append(input(ix).toChar))
```
Two problems. Indexed access into a multi-fragment `ByteString` walks the
fragment list on every byte, and the loop cannot use the JDK's bulk `String`
decode. `ByteStringParserInput.sliceString` already does the fast thing for
exactly the same job - `bytes.slice(start, end).decodeString(ISO_8859_1)` - so
the two were inconsistent.
It also **sign extended**: `input(ix).toChar` turns byte `0xC3` into
`U+FFC3`, not `U+00C3`. Three call sites can see bytes above 0x7F:
- the response reason phrase (`HttpResponseParser:99`) - `scanNewLineIdx`
accepts any byte until CR/LF;
- chunk extensions (`HttpMessageParser:299`) - `parseChunkExtensions`
accepts any byte until CR/LF;
- header names (`HttpHeaderParser:169`) - when
`illegal-response-header-name-processing-mode` is `warn` or `ignore`.
The two call sites in `scanHeaderValue` cannot: `sb` stays `null` only while
every character so far is legal 7-bit ASCII (`' ' <= c <= 0x7F`), so the range
handed to `asciiString` there is always ASCII, where old and new agree exactly.
### Modification
```scala
private[http] def asciiString(input: ByteString, start: Int, end: Int):
String =
if (start == end) "" else input.slice(start, end).decodeString(ISO88591)
```
Reusing the existing `ISO88591` constant from `pekko.http.impl.util`. The
name `asciiString` is kept - callers expect it and most ranges really are ASCII
- with a scaladoc note about the above-0x7F behaviour.
### Result
One bulk decode instead of a per-byte loop, and byte ranges above 0x7F
decode to the Latin-1 character instead of a sign-extended one.
Measured over 3M extractions of a 4-character header name from a
request-sized `ByteString`: **234 ms -> 167 ms**.
Note this is on the *uncached* header path - `HttpHeaderParser` reaches
`asciiString` only when a header is not already in its trie, so for traffic
with repetitive headers most requests never get here. This is an
allocation-pressure and tail-latency improvement rather than something that
moves steady-state throughput.
A pekko-side `decodeString(charset, from, until)` would take this further by
removing the intermediate slice as well (the same measurement puts that at ~108
ms), and this becomes a one-line follow-up if and when that lands. The `slice`
form works on every pekko version in the meantime, including the 1.1.5 that the
1.4.x branch is pinned to.
### Tests
- `sbt "http-core / Test / testOnly
org.apache.pekko.http.impl.engine.parsing.*"` - 242 passed.
- New `ResponseParserSpec` case: a response with a non-ASCII header name,
parsed with `IllegalResponseHeaderNameProcessingMode.Ignore`. It **fails before
this change**, where the two UTF-8 bytes of the name are parsed as the
sign-extended characters `U+FFC3 U+FFB6` instead of `U+00C3 U+00B6`.
- `sbt http-core/mimaReportBinaryIssues` - clean.
- `scalafmt --mode diff-ref=upstream/main` - clean.
### Note on an existing test gap
While writing the test I first tried to assert a non-ASCII *chunk
extension*, which is the other affected path. That turned out to be untestable
through these specs: `StrictEqualHttpResponse` compares entities via
`toStrict`, which collapses a chunked entity to its concatenated data and
discards the extensions. So the existing "message chunk with and without
extension" case does not actually assert the extension strings either. Worth a
separate look, out of scope here.
### References
None - found while auditing ByteString usage across the code base
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
