hsutter commented on code in PR #51270:
URL: https://github.com/apache/arrow/pull/51270#discussion_r3994345071
##########
cpp/src/arrow/acero/test_nodes.cc:
##########
@@ -274,7 +274,7 @@ struct GatedNode : public ExecNode, public TracedNode {
}
GatedNode(ExecPlan* plan, std::vector<ExecNode*> inputs,
- std::shared_ptr<Schema> output_schema, const GatedNodeOptions&
options)
+ const std::shared_ptr<Schema>& output_schema, const
GatedNodeOptions& options)
Review Comment:
Thanks again.
For reading convenience here's the whole function as it is today:
```cpp
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.
--
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]
