-
Notifications
You must be signed in to change notification settings - Fork 580
messages::serialize
: take/put buffers from/into a SerializeBufferPool
#2823
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
Conversation
16838fa
to
0203008
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks reasonable. I'd prefer to have the pool size metric(s) in before we merge, so we can know that the pool isn't growing without bound. For these metrics, I'd recommend:
- Number of buffers in the pool.
- Total size in bytes of all buffers in the pool.
- Number of newly-allocated buffers.
I think these would give us sufficient information to decide if we need to impose a cap, and what that cap should be if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed this yesterday but didn't hit the button - LGTM, this is very straightforward yet effective! Wondering if there's a potential concern with use of a SegQueue, if we happen to allocate a bunch of memory at one point and then it hangs around forever, but choosing the right capacity for an ArrayQueue seems hard.
Is there a benefit between having a global queue and just reusing the same buffer per client? Given it seems that each client would only use 1 or 2 buffers, and then the buffer capacities are also more specialized to the specific client (and also the module) rather than growing towards the global maximum. |
Something like this |
A few benefits to global pools:
That said, the benefit of local buffers is the avoided synchronization overhead. |
0203008
to
de170b6
Compare
de170b6
to
42dc169
Compare
Not sure how this got missed, as it's used in `core/src/startup.rs`. Perhaps a bad merge/rebase?
Description of Changes
Fixes #1858.
In
ws_client_actor_inner
, after havingws.feed(msg)
, keep underlying allocations around, and use a default size of 4KiB.API and ABI breaking changes
None
Expected complexity level and risk
The changes are somewhat straight forward, and it's not a deep change, but it's in a fairly important place.
Testing