hsutter opened a new pull request, #51270: URL: https://github.com/apache/arrow/pull/51270
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
