Eliminate unnecessary refcount traffic from shared_ptr parameters passed by value that don't need to be

[hsutter]

This is a followup to #51147. (Context: I’m experimenting with writing a Claude skill that implements coding guidelines, and I picked "shared_ptr passed by value creating needless refcount inc/dec" because this 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. I reviewed the changes in this PR; this is not a blind dump of unreviewed random AI suggestions.)

Rationale for this change

When a shared_ptr parameter is passed by value but not moved from (or assigned to or otherwise modified), the unused refcount inc/dec traffic is wasted effort. See also #31567, "Overhead of std::shared_ptr<DataType> copies is causing thread contention."

Compilers don't optimize out this extra refcount traffic, so we need to remove it from the code. The least invasive fix is to pass the shared_ptr by const& instead.

What changes are included in this PR?

This PR changes about 400 shared_ptr parameters from pass by value to pass by const&.

I reviewed each change Claude suggested, so any mistakes are my fault.

Are these changes tested?

Only for clean compilation. I'm not familiar enough with the project (sorry) to run tests, especially performance tests (which I hope might improve); this is why I asked for help in #51147, and @pitrou and @rok graciously responded (thanks again!).

The main source of potential bugs I can think of that could be introduced by changing a parameter to pass-by-reference would be if the function body modified the parameter, in which case the change would become a side effect on the caller's argument; obviously that would be bad. This PR prevents that by ensuring all affected parameters are also const, and so the function body would not compile if it tried to modify the affected parameter.

Are there any user-facing changes?

No.

[github-actions]

Thanks for opening a pull request!

This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format.

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[pitrou]

I don't think this is right. MakeIteratorFromReader should instead take its argument by value instead of const-ref.

[hsutter]

@pitrou, thanks again.

Can you help me to understand this feedback please...

1. I'm not sure how MakeIteratorFromReader is related. That's outside the lambda whose parameter is being changed here?

2. The lambda only dereferences batch. Doesn't currently taking batch by value add a (needless?) inc/dec on the refcount? Isn't changing it to pass by const& a strict improvement?

Again, my apologies if I'm missing something here or asking a silly question! I don't know this code base well and I appreciate your feedback.

[pitrou]

This seems wrong too: the ExecNode constructor takes output_schema by value. So the change should instead be to add a std::move.

[hsutter]

Thanks again.

For reading convenience here's the whole function as it is today:

  GatedNode(ExecPlan* plan, std::vector<ExecNode*> inputs,
            std::shared_ptr<Schema> output_schema, const GatedNodeOptions& options)
      : ExecNode(plan, inputs, {"input"}, output_schema),
        TracedNode(this),
        gate_(options.gate) {}

First, I think we agree there's a performance bug? We agree this function should be changed and that there is a needless copy, right? This is a good thing!

Alternative 1 (this PR): The PR's proposed change to pass output_schema by const& will completely eliminate the needless copy:

• Performance: It changes the needless copy to "nothing."
• Readability: Adding const& declares in intent up front (on the declaration) and avoids disturbing the function body (no need to remember to be careful how to use the parameter). Personally I prefer declaring intent as simpler code to read and maintain.

Alternative 2 (add std::move() in the body): If instead we kept pass by value and added a std::move, that would also eliminate the needless copy too, but:

• Performance: It would change a copy to a "move." That's still much cheaper than a copy for shared_ptr, but FWIW a move is still more expensive than "nothing."
• Readability: Adding std::move() at each point of use requires remembering to do that the body (and it seems like we agree it's a problem that the current code doesn't do it, so maybe that's a proof point that it's easy to forget to do it?), and that the reader and maintainer remember that too which is the greater cost over time. IME that's a greater cost than declaring intent on the declaration?

Isn't this PR's suggestion worth considering, to change a copy to nothing at all and with arguably simpler code?

Again, sorry if I'm missing something! (In particular, I have no idea whether these signatures I'm proposing to change might be exported/API functions, e.g., for use in cross-language APIs, that must be pass by value and can't tolerate pass by const&. Is that an issue, and if so how can I tell which types/functions can't tolerate such a parameter passing change so I can exclude them?)

Thank you for your feedback.

[pitrou]

Same here: should move the schema constructor argument into the schema_ attribute instead.

[hsutter]

I would repeat the same considerations as in my reply about GatedNode... I try to make the case that (minor) adding a std::move is slightly less performance and (major) it's less simple/maintainable. Does that position seem reasonable?

[pitrou]

Hi @hsutter , I'm not doing a full review for now. I've just looked at a few changes in diff view order and found deficiencies in how the AI reasoned about the code.

I'm especially surprised that such a simple pattern as passing a constructor arg by value and then moving it to an instance attribute has not been suggested. Also in some cases the intention is clearly to store the shared_ptr for lifetime purposes (for example when returning a lambda or iterator).

I think this shows that a bit more human-driven effort is required to get this to a desirable end state.

[hsutter]

Thanks @pitrou for your time and helpful feedback. I'll close this PR and take another look. Sorry for the noise!

[rok]

@pitrou would it still make sense to benchmark this branch? From your comment I gather no, but would rather check.

[pitrou]

@rok That would be @hsutter 's call.

[rok]

@hsutter running benchmarks is a single github command, happy to kick it of if you think it makes sense.

[hsutter]

Yes please, if it can give a comparison with/without the PR that would be useful. Thank you!

…

________________________________ From: Rok Mihevc ***@***.***> Sent: Thursday, 10 September 2026 04:06:31 To: apache/arrow ***@***.***> Cc: Herb Sutter ***@***.***>; Mention ***@***.***> Subject: Re: [apache/arrow] Eliminate unnecessary refcount traffic from `shared_ptr` parameters passed by value that don't need to be (PR #51270) [https://avatars.githubusercontent.com/u/54589?s=20&v=4]rok left a comment (apache/arrow#51270)<#51270 (comment)> @hsutter<https://github.com/hsutter> running benchmarks is a single github command, happy to kick it of if you think it makes sense. — Reply to this email directly, view it on GitHub<#51270?email_source=notifications&email_token=AANX2NXIHAJBGQB77VUPH335OKDLPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRRG43DQMRRGI22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5617682125>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AANX2NSAXPLR5GPU2V27YWD5OKDLPAVCNFSNUABEKJSXA33TNF2G64TZHM2TCOJQGUZTKMZ3JFZXG5LFHM2TIMBVG4ZTAMJTGKQXMAQ>. Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/AANX2NRPBPXL3XCOFPO7GA35OKDLPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRRG43DQMRRGI22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/AANX2NR2OJOFWNE6ETSP2OT5OKDLPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRRG43DQMRRGI22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>

[hsutter]

I took more time to look at the three review comments, and IMO the PR is actually correct about those three proposed changes. I've tried to reply with some rationale why (note all these replies are my manual analysis and writing, NOT Claude; I just happen to still agree with the changes that my Claude skill generated).

In the meantime, let me reopen this PR -- I closed it to defer to the push-back that the PR wasn't ready, but now that I've reviewed it myself and still agree, I think there is still value in the PR.

However, to repeat a caveat that I wrote above: I have no idea whether these signatures I'm proposing to change might be exported/API functions, e.g., for use in cross-language APIs, that must be pass by value and can't tolerate pass by const&. If that is a constraint for any of the ~400 proposed changes please tell me, and if so please let me know how can I tell which types/functions can't tolerate such a parameter passing change so I can exclude them and make this PR better.

Thanks again for your time and interest!

[zanmato1984]

Hi Herb,

Before getting into the review, I would like to personally thank you for your atomic<> Weapons talk. It helped me tremendously in understanding C++ atomics and the memory model.

I agree with @pitrou that these changes need to distinguish borrowing parameters from ownership sinks. const& is appropriate for borrowing, while taking by value and moving onward allows an ownership sink to accept both lvalues and rvalues efficiently.

I left two additional inline examples in the scalar and JSON reader paths where the current changes introduce a shared_ptr copy for existing rvalue callers. These are particularly relevant because avoiding such reference-count operations is the stated goal of this PR.

I think the changes should be audited using this distinction.

[zanmato1984]

This constructor is an ownership sink rather than a borrowing function. Scalar retains type, and MakeScalarImpl::Visit currently passes std::move(type_) here.

With the previous value parameter, that ownership could be transferred through the constructor chain using moves. With const std::shared_ptr<DataType>&, the rvalue binds to the reference, but Scalar(type, ...) must then copy it into its value parameter. This introduces a reference-count increment and decrement that the previous code avoided.

Could this parameter remain by value and be moved into Scalar?

[zanmato1984]

context is ultimately owned by the returned reader pipeline, so this is an ownership-transfer path rather than a pure borrow.

The caller passes a newly created shared_ptr<DecodeContext>. Previously, the value parameter received that temporary and context = std::move(context) transferred it into the capture without a reference-count operation. After changing the parameter to const&, context = context must copy the shared_ptr.

Could this remain a value parameter with a move capture?

[rok]

@ursabot please benchmark

[rok]

Benchmark runs are scheduled for commit f7556fe. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete.

[rok]

@hsutter unfortunately this currently won't compile so benchmarks cannot be run. Please ping when it does and someone will kick benchmarking off.

[hsutter]

@rok: Aha, thanks -- time for me to quote Homer and say, "D'oh! Always-retain semantics and rvalue arguments." I'll close this again and go rework.

Yes, I was missing the issue of rvalue arguments and sink functions. So what I told Claude is not consistent with what I teach:

Thanks for your patience pointing out this thinko. I had that in an earlier version of the skill and didn't notice I'd removed it, which was a silly error on my part and I should have remembered it here. (Side note: This is a case where I wish C++ had more automatic move-from-definite-last-use of a local variable including a by-value parameter. I still intend to propose that for future C++, and it would help a lot here.)

---

Re building cleanly: Locally, I've been doing cmake --build in arrow/cpp. That seemed to build clean, but I see in the failing jobs there are errors like this that I didn't get in my local build:

/arrow/cpp/src/arrow/csv/reader.cc:1222:38: error: no declaration matches 'arrow::Result<std::shared_ptr<arrow::csv::TableReader> > arrow::csv::TableReader::Make(arrow::io::IOContext, const std::shared_ptr<arrow::io::InputStream>&, const arrow::csv::ReadOptions&, const arrow::csv::ParseOptions&, const arrow::csv::ConvertOptions&)'

Any ideas why I'm not getting these myself when I build locally?

[hsutter]

Before getting into the review, I would like to personally thank you for your atomic<> Weapons talk. It helped me tremendously in understanding C++ atomics and the memory model.

Thanks for the kind words! 🙏 You're most welcome, and I'm glad to know you found it useful.