GitHub user hsutter edited a discussion: Offering to do an experiment if someone helps: Reducing `shared_ptr` refcount traffic, such as pass-by-value?
Hi! Potential first-time contributor here. I noticed[*] that this repo seems to pass refcounted pointers (e.g., `shared_ptr`) by value a lot in ways that seem to incur needless refcount increment/decrement, when the callee does not keep the refcount. For example (these are all either called in loops, or appear to be liable to be frequently used including potentially in hot code though I don’t know the code enough to be sure): - `arrow/acero/unmaterialized_table_internal.h:245`, `AddEntry` parameter `rb` - `arrow/adapters/tensorflow/convert.h:80`, `GetTensorFlowType` parameter `dtype` - `arrow/array/builder_time.h:41` (and also `:56`), `DayTimeIntervalBuilder` (and `MonthDayNanoIntervalBuilder`) parameter `type` - `arrow/compute/kernels/hash_aggregate_pivot.cc:336`, `MergeColumn` parameter `other_column` (called in a loop in line 330) - `arrow/csv/inference_internal.h:105`, lambda function parameter `type` A simple minimal fix we usually recommend to remove the extra refcount traffic would usually be: [**] - If passing the `shared_ptr<T>` parameter by `const&` would compile, do that. - Otherwise, if passing it by `&` would compile, do that. If the function parameter type cannot be changed (e.g., because it’s a virtual override, or its address is taken, or for any other reason needs to keep its current signature), some (most? it depends) of the extra refcount traffic could still be addressed function-internally: - Otherwise, leave it as pass by value but `std::move` on every definite last use of the parameter in the function body (this avoids disturbing the signature). See my CppCon 2022 talk [at 1:16:17 for a quick 1-minute description]( https://youtu.be/ELeZAKCN4tY?si=T_JIBA5QMnRetb9w&t=4577) what I mean by definite last use. Browsing past issues, it seems that a partial tactical removal of `shared_ptr` pass-by-value did happen in 2022, but also that there seems to be continued rounds of discussion since 2016 (e.g., #31567). **Offering a suggested experiment:** If I created a PR that proposes changing a bunch of pass-by-value cases to eliminate needless refcount inc/dec, would someone here who is able to run some performance tests be interested in verifying the proposed changes and how much (if at all) they might help performance? Thank you, Herb [*] Context: I’m a [C++ Core Guidelines](https://github.com/isocpp/CppCoreGuidelines) coauthor currently experimenting with writing a Claude skill that implements coding guidelines. I picked this pitfall because passing a `shared_ptr` by value when the callee does not keep a refcount has always been the top performance pitfall of using `shared_ptr`. When I asked Claude to list some popular GitHub repos that seemed to have a lot of violations, `apache/arrow` was one of the top five Claude flagged, and as I’m manually checking the skill’s output for this repo the issues it flagged appear to be real so far. [**] I would _not_ propose the “idealistic in new code” change of passing the `T` directly by `T&` or `T*`. In new code that’s better for a function that doesn’t need the `shared_ptr`-ness and only needs the `T`. However, as a pragmatic change to existing code, it would make the change much more invasive (and costly) for no actual additional performance benefit. GitHub link: https://github.com/apache/arrow/discussions/51147 ---- This is an automatically sent email for [email protected]. To unsubscribe, please send an email to: [email protected]
