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

Investigate leveraging ArrayPool #6710

Closed
4 tasks done
Tracked by #6940
rokonec opened this issue Jul 26, 2021 · 6 comments
Closed
4 tasks done
Tracked by #6940

Investigate leveraging ArrayPool #6710

rokonec opened this issue Jul 26, 2021 · 6 comments

Comments

@rokonec
Copy link
Contributor

rokonec commented Jul 26, 2021

We are introducing ArrayPool in #6705.

By quick analyze I have identified following areas which might leverage from buffers pool.

  • InterningBinaryReader._buffer - Since _buffer is used only in ReadString we can simplify code, delete _buffer and its initialization from call stacks and just Rent\Return buffer of destination string size in the ReadString method.
  • Avoid BinaryReader.ReadBytes and related Translate(ref byte[] byteArray) - good candidate is TargetResult.TranslateItems
  • Consider using RecyclableMemoryStream (or some equivalent using ArrayPool.Shared) in places where our current usage of MemoryStream is allocating a lot.
  • Others - yet to be identified.
@rokonec rokonec added performance Performance-Scenario-General This issue affects performance in general. needs-triage Have yet to determine what bucket this goes in. labels Jul 26, 2021
@benvillalobos benvillalobos removed the needs-triage Have yet to determine what bucket this goes in. label Jul 29, 2021
@AR-May AR-May added the backlog label Aug 9, 2021
@ladipro ladipro added the size:1 label Oct 12, 2021
@ladipro ladipro changed the title Leverage ArrayPool Investigate leveraging ArrayPool Oct 12, 2021
@ladipro ladipro self-assigned this Oct 27, 2021
@ladipro ladipro assigned rokonec and unassigned ladipro Nov 8, 2021
@rokonec
Copy link
Contributor Author

rokonec commented Nov 12, 2021

Although we can replace public void Translate(ref byte[] byteArray) by public void Translate(ref byte[] byteArray, ref int length) its actual allocation portion is < 0.09%

@rokonec
Copy link
Contributor Author

rokonec commented Nov 12, 2021

Using RecyclableMemoryStream would not bring as much gain as originally though. Our code already uses somehow shared MemoryStream in most hot places.

We would more leverage from using reusable BinaryReader and/or BinaryWritter as we create both classes for every packet and both classes lazy allocates couple of working buffers.

@rokonec
Copy link
Contributor Author

rokonec commented Nov 12, 2021

I have not identified other places where using ArrayPool would bring measurable performance benefit.

@rokonec rokonec moved this from To Do to In Review in Perf sprint Nov 8th - Nov 21st 2021 Nov 12, 2021
@rokonec rokonec assigned ladipro and unassigned ladipro Nov 12, 2021
@rokonec
Copy link
Contributor Author

rokonec commented Nov 12, 2021

@ladipro @AR-May
Please check my conclusion. Lets discuss sometimes if we want to invest into reusable BinaryReader and/or BinaryWritter as started above.

@ladipro
Copy link
Member

ladipro commented Nov 15, 2021

@rokonec thank you, what you wrote makes perfect sense. Is it possible to quantify the impact of building a reusable BinaryReader and BinaryWriter? In terms of % of allocations saved when building a large solution, or something similar.

@rokonec
Copy link
Contributor Author

rokonec commented Nov 15, 2021

@ladipro I was mislead by dotMemory tool I am evaluating. I expected percentage was from overall memory but it was from given type - in this case byte[]
I have measured it again and and it is bellow 0.1% of memory usage I do not longer recommend to invest into it.

Sorry for this mistake.

@rokonec rokonec closed this as completed Nov 15, 2021
@ladipro ladipro moved this from In Review to Done in Perf sprint Nov 8th - Nov 21st 2021 Nov 16, 2021
@ladipro ladipro added this to the VS 17.1 milestone Dec 8, 2021
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants