armeria icon indicating copy to clipboard operation
armeria copied to clipboard

[WIP] Introduce `CompositeHttpHeadersBase` to replace HttpHeadersUtil.merge()

Open injae-kim opened this issue 1 year ago โ€ข 4 comments

related issue: https://github.com/line/armeria/issues/5146

Motivation:

https://github.com/line/armeria/blob/3595dacd9e7675fec309a1029a2a97b829c08682/core/src/main/java/com/linecorp/armeria/internal/common/HttpHeadersUtil.java#L46-L46

  • On https://github.com/line/armeria/issues/5146, we found that HttpHeadersUtil.mergeHeaders(..) occupies a large part of CPU cycles due to expensive deep-copy operations

๐Ÿ“Œ PRs Overview:

  1. โœ… (this PR) Introduce CompositeHttpHeadersBase to replace HttpHeadersUtil.mergeHeaders(..)
  2. Introduce CompositeHttpBeaderBuilder builder
  3. Introduce DefaultCompositeRequest, ResponseHeaders
  4. (finally) Replace HttpHeadersUtil.mergeHeaders(..) to Composite*Headers

Modifications:

  • Introduce CompositeHttpHeadersBase and CompositeStringMultiMap to replace HttpHeadersUtil.merge()

Result:

  • Now we have CompositeHttpHeadersBase so can implement DefaultCompositeRequest, ResponseHeaders

injae-kim avatar Dec 08 '23 13:12 injae-kim

๐Ÿ” Build Scanยฎ (commit: e9fcd38353a5cbfa133bcc3992ca36cf8feb8eb6)

Job name Status Build Scanยฎ
build-windows-latest-jdk-19 โœ… https://ge.armeria.dev/s/fagki3cgeavso
build-self-hosted-unsafe-jdk-8 โœ… https://ge.armeria.dev/s/7zm72vtzosdem
build-self-hosted-unsafe-jdk-19-snapshot-blockhound โœ… https://ge.armeria.dev/s/ry56m32qmgk56
build-self-hosted-unsafe-jdk-17-min-java-17-coverage โœ… https://ge.armeria.dev/s/w5ttm4bkqh7jm
build-self-hosted-unsafe-jdk-17-min-java-11 โŒ (failure) https://ge.armeria.dev/s/qevvxewiy5ptw
build-self-hosted-unsafe-jdk-17-leak โœ… https://ge.armeria.dev/s/cf22vfdohvxbq
build-self-hosted-unsafe-jdk-11 โœ… https://ge.armeria.dev/s/cmfbbdk3iuo4e
build-macos-12-jdk-19 โœ… https://ge.armeria.dev/s/zkj54c54wbns4

github-actions[bot] avatar Dec 08 '23 14:12 github-actions[bot]

Codecov Report

Attention: Patch coverage is 84.47552% with 111 lines in your changes missing coverage. Please review.

Project coverage is 74.21%. Comparing base (75d5af1) to head (e9fcd38). Report is 194 commits behind head on main.

Files Patch % Lines
...necorp/armeria/common/CompositeStringMultimap.java 84.67% 52 Missing and 45 partials :warning:
...ecorp/armeria/common/CompositeHttpHeadersBase.java 82.92% 9 Missing and 5 partials :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5340       +/-   ##
===========================================
+ Coverage        0   74.21%   +74.21%     
- Complexity      0    21119    +21119     
===========================================
  Files           0     1803     +1803     
  Lines           0    77283    +77283     
  Branches        0     9948     +9948     
===========================================
+ Hits            0    57354    +57354     
- Misses          0    15254    +15254     
- Partials        0     4675     +4675     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 08 '23 14:12 codecov[bot]

Really sorry for late, I'll update this PR within this weekend ๐Ÿ™‡

injae-kim avatar Feb 01 '24 06:02 injae-kim

Really sorry for late, I think we can reduce complexity of CompositeHeader by supporting only add with zero-copy.

  • If we support zero-copy on add, set, remove, logic is too complex and easy to make bug
    • If we usually don't set, remove after mergeHeaders, we can minimize complexity of CompositeHeader

I found that spring has similar CompositeAttribute impl but it has many bugs.

โœ… So, I'll check mergeHeaders's usage and prepare benchmark test too~! ๐Ÿ™‡

injae-kim avatar Jul 04 '24 15:07 injae-kim