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

Workaround allocation regression caused by missing specialized [Substring].joined #161

Conversation

simonjbeaumont
Copy link
Contributor

Motivation

The specialization for [Substring].joined was dropped in Swift 5.91 and consequently we regressed in allocations2.

Modifications

Workaround the lack of specialized [Substring].joined by handwriting the comma-separated concatenation of the SSH algorithms.

Result

Reduces the allocations to below what they were prior to the toolchain regression.

Notes

As a baseline, I was using d23c142 of this repo, which is a commit from around the time of the toolchain regression.

The allocation results are as follows:

swift-nio-ssh swift toolchain allocs
d23c142 (Aug 11) 5.9-2023-07-25 1,007,000
5.9-2023-07-29 1,100,000
5.9-RELEASE 1,100,000
d23c142 + patch 5.9-2023-07-25 989,000
5.9-2023-07-29 989,000
5.9-RELEASE 989,000
d33c701 (main) 5.9-RELEASE 1,060,000
main + patch 5.9-RELEASE 949,000

From this we infer:

  1. The allocation regression was caused by a regression in Swift 5.9 between nightly versions 2023-07-25 and 2023-07-29.
  2. The same regression is still present in 5.9-RELEASE.
  3. The patch seems to mitigate the regression we're seeing as a result of the new 5.9 behavior.
  4. Since the original regression we actually saved some 40k allocations by some other change in the code (either here, or more likely in a dependency).

For more information on the Swift issue, including reproducer package and heaptrack outputs, see the Swift issue1.

Footnotes

  1. https://github.com/apple/swift/issues/69883 2

  2. Update allocation results #157

@dnadoba
Copy link
Member

dnadoba commented Nov 15, 2023

@swift-server-bot add to allowlist

@simonjbeaumont
Copy link
Contributor Author

CI expected to fail because allocations are now much less than the figure in the CI config. The runs from the above commit show these observed allocations:

  • 5.7: 952,0001
  • 5.8: 939,0002
  • 5.9: 949,0003
  • nightly: 939,0004

Weirdly, 5.10 failed because the allocations for a different tests were out of the allowed range5:

16:06:14 ++ echo 'info: client_server_many_small_commands_per_connection: total number of mallocs: 193748'

Footnotes

  1. https://ci.swiftserver.group/job/swift-nio-ssh-swift57-prb/137/console

  2. https://ci.swiftserver.group/job/swift-nio-ssh-swift58-prb/66/console

  3. https://ci.swiftserver.group/job/swift-nio-ssh-swift59-prb/48/console

  4. https://ci.swiftserver.group/job/swift-nio-ssh-nightly-prb/211/console

  5. https://ci.swiftserver.group/job/swift-nio-ssh-swift510-prb/2/console

@glbrntt glbrntt added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Nov 16, 2023
@simonjbeaumont simonjbeaumont marked this pull request as ready for review November 16, 2023 09:04
@simonjbeaumont simonjbeaumont merged commit ef94274 into apple:main Nov 16, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants