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

Always use ArrayPool.Shared for optimized use of buffers in buffer manager #2224

Merged
merged 4 commits into from
Jul 22, 2023

Conversation

marcschier
Copy link
Contributor

  • The suggestion from .net team is to use shared pool if you have < 1mb buffers you need.
  • We now share buffers across connections which is far more efficient when many channels are created and closed.
  • There should never be buffers > 64k per spec. But for the memory tracking I have left the old code.
  • Open issue. There is no need to lock (pin?) the memory so this was removed because it added 1 additional byte and then you always allocated far larger buffers (likely 128k) for every 64k you wanted. The tracking code is still the same.

@marcschier
Copy link
Contributor Author

It was remarked that Buffer manager is only created once. That is correct for server, but for client it is created every channel (so twice for every session establishment).

Also, please see the documentation on ArrayPool, if you try to rent a buffer that is larger than the max configured (64k + 1) then it always allocates a fresh buffer. If you return this buffer, it is likely not added to a bucket as there is none. Even if that is working on one version of .net it might be different on another.

Finally, even if you were to leave the code as is and increase the buffer size by 1 the allocated buffers in the pool will likely be the next highest bucket size, which is very wasteful. What is worse, this probably pushes all these allocations into LOH (> 85k) which is not good.

@mregen
Copy link
Contributor

mregen commented Jul 14, 2023

@marcschier agree for perf reason to use the shared version, but please bring the cookies back...

@mregen mregen self-requested a review July 14, 2023 04:43
Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

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

see comments..

m_arrayPool = maxBufferSize <= 1024 * 1024
? ArrayPool<byte>.Shared
: ArrayPool<byte>.Create(maxBufferSize, 32);
#if TRACK_MEMORY
Copy link
Contributor

Choose a reason for hiding this comment

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

use maxArrayLength, bring cookie back etc. Its needed for runtime check of buffer usage.

@mregen
Copy link
Contributor

mregen commented Jul 17, 2023

.shared has a couple of advantages:
i) the allocator uses GC.AllocateUnitializedMemory vs. new with Create
ii) there are global and thread local buckets vs. fixed size with Create
iii) the pool is hooked up to GC Trim callbacks, allows to free up buckets if not in use based on memory pressure vs. fixed bucket size / no trim with Create

@mregen
Copy link
Contributor

mregen commented Jul 17, 2023

just as a sidenote, the existing code uses 65535 size buffers and ands 1 byte cookie, so the allocations are still in the 64k bucket.
Althought it is a bug to initialize the pool without taking the cookie into account, since it is still in the bucket size it was luckily not causing memory exhaust.

@mregen mregen self-requested a review July 17, 2023 09:22
@mregen mregen merged commit 7f85159 into master Jul 22, 2023
70 of 72 checks passed
TimJoehnk pushed a commit to TimJoehnk/UA-.NETStandard that referenced this pull request Aug 4, 2023
…nager (OPCFoundation#2224)

* The suggestion from .net team is to use shared pool if you have < 1mb buffers you need. 
* We now share buffers across connections which is far more efficient when many channels are created and closed.
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.

None yet

2 participants