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

Reduces heap allocations for the some byte[] uses #1272

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

jacobslusser
Copy link
Contributor

Profiling reveals that creation of byte[4] arrays are our most common heap objects. Many of these come from reading integers from the byte stream where we allocate a small byte[4] or byte[8] array and then convert it to a 32 or 64-bit integer.

Results from a simple test of using the SftpClient to upload and download some files:

Before: when byte[] arrays were created for each integer read
Screenshot 2023-12-10 181955

After: when byte[] are allocated and read from the stack
Screenshot 2023-12-10 182136

Changes

  • Creates a new SshDataStream.ReadBytes overload that supports Span<byte> for .NET Standard 2.1+ and .NET 6+ targets.
  • Updates SshDataStream.ReadUInt32 and SshDataStream.ReadUInt64 to read onto the stack and use newer BinaryPrimitives class for integer conversion for .NET Standard 2.1+ and .NET 6+ targets.
  • Updates SshData.ReadUInt32 and SshData.ReadUInt64 to use the existing stream methods to avoid another temporary buffer.

@Rob-Hague
Copy link
Collaborator

Funny, I had very similar changes in #1138 🙂 Feel free to cherry-pick some of those, or otherwise this looks good to me

@WojciechNagorski WojciechNagorski merged commit f4371ff into sshnet:develop Dec 11, 2023
1 check passed
@jacobslusser
Copy link
Contributor Author

Funny, I had very similar changes in #1138 🙂 Feel free to cherry-pick some of those, or otherwise this looks good to me

@Rob-Hague wow, your changes are more than a little similar. At one point I had even ported the ReadExactly just as you had but decided instead to follow our existing pattern. I will definitely look at merging the others.

Why didn't you ever merge this?

@Rob-Hague
Copy link
Collaborator

I think I wanted to justify it with some profiling like you did, but got stuck into some other work

@WojciechNagorski
Copy link
Collaborator

I will merge any optimization if it has justify as goof as this PR 😃. #1138 was a draft.

@WojciechNagorski
Copy link
Collaborator

The 2023.0.1 version has been released to Nuget: https://www.nuget.org/packages/SSH.NET/2023.0.1

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.

3 participants