Avoid unnecessary parsing of content-type header in ServerInboundHandler
#2644
Conversation
kyri-petrou
requested review from
jdegoes,
vigoo,
adamgfraser and
987Nabil
as code owners
kyri-petrou
changed the title
content-type header in ServerInboundHandlercontent-type header in ServerInboundHandler
|
|
||
| override def contentType(newMediaType: MediaType, newBoundary: Boundary): Body = | ||
| copy(mediaType = Some(newMediaType), boundary = boundary.orElse(Some(newBoundary))) | ||
| copy(mediaType0 = () => Some(newMediaType), boundary0 = () => boundary.orElse(Some(newBoundary))) |
There was a problem hiding this comment.
By the way, is this the correct behaviour? I'm a bit confused on why we would keep the old boundary if it was defined. Shouldn't this be boundary = Some(newBoundary) instead?
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2644 +/- ##
==========================================
- Coverage 65.38% 65.34% -0.05%
==========================================
Files 144 144
Lines 8407 8414 +7
Branches 1532 1486 -46
==========================================
+ Hits 5497 5498 +1
- Misses 2910 2916 +6 ☔ View full report in Codecov by Sentry. |
|
The test failure is due to a flaky test ScalaJS. I opened an issue for it; I tried rerunning the entire test suite via an empty commit but the test failed again so it's perhaps better to just restart the failed run |
| contentTypeHeader.map(_.mediaType), | ||
| contentTypeHeader.flatMap(_.boundary), | ||
| ) | ||
| contentTypeHeader: () => Option[Header.ContentType] = () => None, |
There was a problem hiding this comment.
Note that allocation of Function0 can be measured in benchmarks.
I would suggest the following:
- Statically make
Header.ContentTypefor common content types (e.g.Header.ContentType.application/json`) - Use fast comparison (or maybe hash map lookup) to decide which
Header.ContentTypeto map to which standard Netty header. If Netty reuses the same char sequence, it could be even faster (eq).
If done properly this can as fast as Function0 and will also be less disruptive to the code.
There was a problem hiding this comment.
Oh also we can do:
Header.ContentType.`some-application/json` to pre-allocate the option.
| asciiString: AsciiString, | ||
| override val mediaType: Option[MediaType] = None, | ||
| override val boundary: Option[Boundary] = None, | ||
| private val mediaType0: () => Option[MediaType] = () => None, |
There was a problem hiding this comment.
Same story here for MediaType. It can also eliminate the Function0 allocation.
| override val mediaType: Option[MediaType] = None, | ||
| override val boundary: Option[Boundary] = None, | ||
| private val mediaType0: () => Option[MediaType] = () => None, | ||
| private val boundary0: () => Option[Boundary] = () => None, |
There was a problem hiding this comment.
Boundary type is tricky because it comes in dynamically. However, it would be nice to eliminate the more complex API here and the Function0 allocation. Did you measure to see if this has any impact?
|
@jdegoes thanks for the feedback. While reading though your comments, I realised that we already do cache media types (via a hash map). I reverted all the With the changes in the latest commit, the performance is slightly improved (compared to the previous commit), but the biggest benefit is that the API is much cleaner The only "downside" to these changes is the use of In addition, I shamelessly broke binary / source compatibility of the |
|
PS: Build is failed due to Sonatype 502 error. Should work after a restart |
While #2597 improved the performance of parsing the content-type header significantly, it is still a relatively expensive computation (w.r.t everything else) so if we can avoid it, why not? While most applications won't see any measurable difference with this change, I'm certain it'll make a difference on benchmarks that include the content-type header as part of the request.
To demonstrate the difference, I added a benchmark in
ServerInboundHandlerBenchmarkthat includes the content type header:Before:
After:
I rerun the benchmark 5-10 times and it seems that avoiding the parsing of the content-type header is consistently 5-10% faster