Offering to do an experiment if someone helps: Reducing shared_ptr refcount traffic, such as pass-by-value?

[hsutter]

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 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 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.

[pitrou]

Hi! It's great to see you interested in Arrow.

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):

These examples look either not hot, or they are batched computation functions where the shared_ptr copy cost would probably be dominated by the overall computation cost (depending on the input Array / RecordBatch size).

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?

We have a benchmarking suite so ideally we can do that, but I think we ran out of AWS credits. 🫤 (@rok is that right?)

That said, even without any significant perf improvements, small harmless code-level improvements can go in anyway.

[rok]

Great to see this interest and such an experiment would be great!

Re benchmarking - we have not shut down yet and even if we do we'll bring it as back soon as we can. Here's an example benchmark and the entire dashboard.