memorypool: remove custom arc in favor of system allocator #48296
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Like our custom
Rc, we have a customArcthat works with a preallocated memory pool. After lookng into the performance of our customRcvs std, I believe our customArccan't be any faster than simply using the stdArcwith the system allocator, and we should remove it. This PR does that, replacing all usage with std.I didn't benchmark our
Arcagainst std, but it's enough to compare the code. Std uses raw pointers (notablyDerefis a direct pointer deref) whereas we protect all operations with aMutex. Given that ourRc's earlier use ofRefCellwas too much overhead to compete with std, aMutexhas no chance. Further, ourArcis currently only used to manage instances ofzmq::Messagewhich is a small Vec-like type, and the system allocator performs very well with small types, at least on Linux with glibc. So, it should be safe to switch to std without consequence.Yes, possibly our
Arccould be optimized to use raw pointers like I did with ourRc. However,Arcis much harder to get right, and the benefits are unclear for our current usage (small types). Removal is an easier solution.We could potentially change our
Arcto wrap std instead of removing it, to make it easier to reintroduce pooling in the future. However, since we are currently only poolingzmq::Messageand not the (more significant) actual content of those messages, my sense is if somday we want to do pooling in this area of the code then it deserves a full rethink anyway, and so in the present I lean toward removal.