Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-43258: [C++][Flight] Use a Base CRTP type for the types used in RPC calls #43255

Merged
merged 13 commits into from
Jul 19, 2024

Conversation

felipecrv
Copy link
Contributor

@felipecrv felipecrv commented Jul 15, 2024

Rationale for this change

Flight should eventually be buildable without a specific protobuf version. As such, all the protobuf types are wrapped in hand-written classes. Uniformity of the interface is not enforced even though it's desirable. Extending the interface requires adding functions to every struct. A common base class can mitigates risks and reduce the amount of hand-written code.

What changes are included in this PR?

  • Definition of a BaseType<> template (a CRTP)
  • Add constructors that aren't implicitly defined anymore
  • Having a default value for some fields that would be undefined values otherwise
  • Leverage this structure to add SerializeToBuffer to all the types
  • ~30KiB in binary size reduction

Are these changes tested?

By existing tests.

@felipecrv felipecrv requested a review from lidavidm as a code owner July 15, 2024 15:42
@felipecrv felipecrv changed the title Flight crtp GH-43258: [C++][Flight] Use a Base CRTP type for the types used in RPC calls Jul 15, 2024
@apache apache deleted a comment from github-actions bot Jul 15, 2024
/// \brief Serialize this message to its wire-format representation.
arrow::Result<std::string> SerializeToString() const;
arrow::Status DoSerializeToString(std::string* out) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the impls protected and/or update the docstrings to make it clear that these are internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making them private breaks self()->Do... calls in the CRTP base class. I could work around this with friend annotations, but to be frank, it was my intention to have these as public functions as well because if I ever deserialize object in a loop, I can re-use an std::string and avoid mallocs.

I will add docstrings telling users that they can call Seria... because they are more convenient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really rather not have so many different methods. If we're going to keep them, please at least make them overloads (so drop the 'Do') since they are just the same methods implemented in terms of each other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reiterating the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will drop the Do- prefix.

I'd really rather not have so many different methods.

If I'm going to automatically generate code (for FlightSQL), the functions that take std::string* produce much less binary bloat than Result<T> which is why this is the pattern adopted in protobuf itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good to me.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 16, 2024
@felipecrv felipecrv requested a review from lidavidm July 18, 2024 01:05
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jul 18, 2024
@felipecrv
Copy link
Contributor Author

bloaty report:

$ ninja libarrow_flight.a && bloaty -d symbols -C full -n 0 ./release/libarrow_flight.a -- BEFORE-libarrow_flight.a
[17/17] Linking CXX static library release/libarrow_flight.a
    FILE SIZE        VM SIZE
 --------------  --------------
  -1.0% -28.1Ki  [ = ]       0    [AR Non-ELF Member File]
  -1.0% -28.1Ki  [ = ]       0    TOTAL

It's small compared to the gigantic arrow library, but any reduction is good IMO.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 19, 2024
@felipecrv felipecrv merged commit 9fb1129 into apache:main Jul 19, 2024
36 of 39 checks passed
@felipecrv felipecrv removed the awaiting merge Awaiting merge label Jul 19, 2024
@felipecrv felipecrv deleted the flight_crtp branch July 19, 2024 02:18
@felipecrv
Copy link
Contributor Author

felipecrv commented Jul 19, 2024

I think I broke the MSVC build by merging this. I will try fixing it here -> #43330 #43332

Copy link

After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 9fb1129.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants