pjfanning opened a new pull request, #1230:
URL: https://github.com/apache/pekko-http/pull/1230
### Motivation
`StringTools.asciiStringFromBytes` uses the deprecated `new String(byte[],
int hibyte)` behind a `@nowarn`, justified by this comment:
> Deprecated constructor but also (unfortunately) the fastest way to convert
a ASCII encoded byte array into a String without extra copying.
That was true on Java 8, where the alternative expanded the bytes into a
`char[]`. It stopped being true with JDK 9 and compact strings, and this branch
requires JDK 17.
The method sits on the HTTP/2 HPACK decode path
(`shaded/com/twitter/hpack/Decoder.readStringLiteral`), so it is worth being
sure the replacement changes neither behaviour nor performance.
### Modification
```scala
- @nowarn("msg=deprecated")
def asciiStringFromBytes(bytes: Array[Byte]): String =
- // Deprecated constructor but also (unfortunately) the fastest way to
convert a ASCII encoded byte array
- // into a String without extra copying.
- new String(bytes, 0)
+ // ISO-8859-1 rather than US-ASCII: this maps every byte to the
character of the same value, which
+ // is what the deprecated `new String(bytes, 0)` this replaces did.
Since JDK 9 (compact strings) it
+ // keeps the array as is with a LATIN1 coder, so it is the same single
copy.
+ new String(bytes, ISO88591)
```
plus dropping the now-unused `scala.annotation.nowarn` import. `ISO88591` is
the constant already defined in this package (`impl/util/package.scala`).
**ISO-8859-1, not US-ASCII**, despite the method name. `new String(bytes,
0)` sets `char = byte & 0xFF` for every byte; ISO-8859-1 does exactly the same,
so the two agree on every possible input. US-ASCII would turn bytes above 0x7F
into `U+FFFD`, and HPACK string literals are opaque octets (RFC 7541), so such
bytes really can arrive here. This matches the same decision made for
`asciiString` in the parser.
### Result
No deprecated JDK API, no `@nowarn`, and no misleading Java-8-era comment,
with identical behaviour.
I did not want to take the "compact strings make these identical" reasoning
on trust, so:
- **Equivalence**: verified in jshell that both forms produce equal
`String`s for all 256 byte values.
- **Performance**: JMH, average time, sizes 12 / 64 / 4096 bytes.
| | 12 B | 64 B | 4096 B |
|---|---|---|---|
| `new String(bytes, 0)` | 25.3 ± 7.0 ns | 30.2 ± 10.7 ns | 1260.2 ± 959.8
ns |
| `new String(bytes, ISO_8859_1)` | 28.2 ± 30.3 ns | 43.1 ± 72.5 ns | 952.7
± 171.4 ns |
Error bars overlap at every size, so there is no measurable difference in
either direction. (A crude jshell loop first suggested the charset form was ~2x
slower at small sizes; that turned out to be warmup/ordering noise, which is
why this was re-measured under JMH.)
### Tests
- New `StringToolsSpec` pins the byte-to-character mapping across the whole
0x00-0xFF range - the property that would break if someone later "corrected"
the charset to US-ASCII to match the method name - plus a round-trip through
`asciiStringBytes` and the empty-array case. `sbt "http-core / Test / testOnly
org.apache.pekko.http.impl.util.StringToolsSpec"` - 4 passed. It is a
regression guard, not a failing-before test; this change is deliberately
behaviour preserving, so nothing can fail against it.
- `sbt http-core/mimaReportBinaryIssues` - clean.
- `scalafmt --mode diff-ref=upstream/main` - clean.
- I did not run the `http2-tests` suite that exercises the HPACK codec end
to end; leaving that to CI.
### References
None - found while reviewing the code base against the JDK 17 baseline
--
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]
