Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for kotlinx-io byte channel handling #4070

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

bjhham
Copy link
Contributor

@bjhham bjhham commented Jun 10, 2024

Addressing comments and CI problems in #4032

@bjhham bjhham requested a review from e5l June 10, 2024 14:32
@bjhham
Copy link
Contributor Author

bjhham commented Jun 10, 2024

The tests pass! Still need to address the common target failing in the allocation tests build and some styling, so it seems.

@bjhham bjhham force-pushed the bjhham/kotlinx-io-3 branch from 128bcb3 to 28cd246 Compare June 10, 2024 14:34
@bjhham bjhham force-pushed the bjhham/kotlinx-io-3 branch from 8384f76 to d325c82 Compare June 10, 2024 15:48
@bjhham
Copy link
Contributor Author

bjhham commented Jun 10, 2024

See this for the allocation tests fix ktorio/ktor-benchmarks#19

Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except some failing tests (eadChannel should not lock file pre read()[jvm]) on windows.

Please also update an API dump

@bjhham
Copy link
Contributor Author

bjhham commented Jun 11, 2024

Seems the allocation test results warrant some investigation...

Netty

Request consumes 484.33 KB, expected 30.94 KB. Difference: 453.39 KB
  Consumed 484.33 KB on request
  Expected 30.94 KB on request
  Extra    453.39 KB on request
(See stdout + build/allocations/* files for details)
Increased locations:
  OutputArraysJVM.kt                      217476.56 KB    (0            --> 217476.56 KB)
  LookAheadSession.kt                     213239.80 KB    (0            --> 213239.80 KB)
  AwaitingSlot.kt                         15679.63 KB    (0            -->  15679.63 KB)
  WriteSuspendSession.kt                  9179.69 KB    (0            -->   9179.69 KB)
  ByteReadPacket.kt                       2531.25 KB    (0            -->   2531.25 KB)
  ByteChannel.kt                          2494.69 KB    (328.13 KB    -->   2822.81 KB)
  ByteWriteChannelOperations.kt            892.77 KB    (0            -->    892.77 KB)
  StaticContentResolution.kt               234.38 KB    (976.56 KB    -->   1210.94 KB)
  ByteReadChannelOperations.kt             234.38 KB    (0            -->    234.38 KB)
  ByteWriteChannel.kt                      140.63 KB    (0            -->    140.63 KB)
  Pipeline.kt                              136.48 KB    (465.08 KB    -->    601.56 KB)
  RequestBodyHandler.kt                     88.23 KB    (426.34 KB    -->    514.58 KB)
  FileChannels.kt                           85.94 KB    (671.88 KB    -->    757.81 KB)
  LocalFileContent.kt                       54.69 KB    (70.31 KB     -->    125.00 KB)
  NettyApplicationResponse.kt               46.88 KB    (101.56 KB    -->    148.44 KB)
  PreCompressed.kt                          39.06 KB    (179.69 KB    -->    218.75 KB)
  ByteReadChannel.kt                        39.06 KB    (0            -->     39.06 KB)

CIO

Request consumes 513.66 KB, expected 166.02 KB. Difference: 347.64 KB
  Consumed 513.66 KB on request
  Expected 166.02 KB on request
  Extra    347.64 KB on request
(See stdout + build/allocations/* files for details)
Increased locations:
  LookAheadSession.kt                     214565.03 KB    (0            --> 214565.03 KB)
  OutputArraysJVM.kt                      209244.67 KB    (0            --> 209244.67 KB)
  AwaitingSlot.kt                         30426.99 KB    (0            -->  30426.99 KB)
  ByteWriteChannelOperations.jvm.kt       16187.02 KB    (0            -->  16187.02 KB)
  WriteSuspendSession.kt                  9179.69 KB    (0            -->   9179.69 KB)
  ByteChannel.kt                          7514.75 KB    (328.13 KB    -->   7842.88 KB)
  ByteReadPacket.kt                       2625.00 KB    (0            -->   2625.00 KB)
  ByteReadChannelOperations.kt            1656.25 KB    (0            -->   1656.25 KB)
  ByteWriteChannelOperations.kt            843.78 KB    (0            -->    843.78 KB)
  ServerPipeline.kt                        554.69 KB    (1056.80 KB   -->   1611.49 KB)
  StaticContentResolution.kt               234.38 KB    (976.56 KB    -->   1210.94 KB)
  Pipeline.kt                              215.00 KB    (414.06 KB    -->    629.06 KB)
  Strings.kt                               193.31 KB    (0            -->    193.31 KB)
  ActorSelectorManager.kt                  164.06 KB    (414.06 KB    -->    578.13 KB)
  FileChannels.kt                           85.94 KB    (671.88 KB    -->    757.81 KB)
  SelectorManagerSupport.kt                 54.69 KB    (195.31 KB    -->    250.00 KB)
  LocalFileContent.kt                       54.69 KB    (70.31 KB     -->    125.00 KB)
  ByteWriteChannel.kt                       46.88 KB    (0            -->     46.88 KB)
  PreCompressed.kt                          39.06 KB    (179.69 KB    -->    218.75 KB)
  ByteReadChannel.kt                        39.06 KB    (0            -->     39.06 KB)
  Text.kt                                   23.44 KB    (726.56 KB    -->    750.00 KB)
  CIOApplicationRequest.kt                  15.63 KB    (226.56 KB    -->    242.19 KB)
  CollectionsJvm.kt                         15.63 KB    (0            -->     15.63 KB)
  SuspendFunctionGun.kt                      1.62 KB    (812.50 KB    -->    814.12 KB)

@e5l
Copy link
Member

e5l commented Jun 11, 2024

Yep, we are. Some places (like LookAheadSession.kt) we can deprecate and replace with a different API

@bjhham bjhham force-pushed the bjhham/kotlinx-io-3 branch from e025845 to 3fc9b7e Compare June 11, 2024 10:27
@bjhham
Copy link
Contributor Author

bjhham commented Jun 11, 2024

I'll merge what I've got for now since the memory footprint testing will take some time to resolve.

@bjhham bjhham merged commit 3f9e70c into e5l/kotlinx-io-2 Jun 11, 2024
2 of 16 checks passed
@bjhham bjhham deleted the bjhham/kotlinx-io-3 branch June 11, 2024 11:35
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