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.

Sign up for GitHub

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

Jump to bottom

[C++] Reduce string inlining in Substrait serde #41141

Open
westonpace opened this issue Apr 11, 2024 · 0 comments
Open

[C++] Reduce string inlining in Substrait serde #41141

westonpace opened this issue Apr 11, 2024 · 0 comments
Assignees
@westonpace
Labels
Component: C++ good-first-issue Type: enhancement

Comments

@westonpace
Copy link
Member

Describe the enhancement requested

Comment from review of #40696

nit: a function to create this Status taking arguments like "integer literal" and "UInt64Value" would reduce the string literal bloat in the binary because "Failed to unpack user defined " wouldn't have to be inlined so many times in the literals.

Adding ARROW_PREDICT_FALSE to the condition would also reduce the inlining of code in the block because it becomes cold code code for the compiler.

These changes could be done throughout the file to improve the performance of the Substrait serde.

Component(s)

C++
@westonpace westonpace changed the title [C++] Reduce string inlining [C++] Reduce string inlining in Substrait serde Apr 11, 2024
@westonpace westonpace self-assigned this Apr 11, 2024
@westonpace westonpace mentioned this issue Apr 11, 2024
Merged
attilajeges added a commit to attilajeges/arrow that referenced this issue Jan 4, 2025
@attilajeges
apacheGH-41141 [C++] Reduce string inlining in Substrait serde
c6caae5 
Rationale for this change:

This patch adds a helper function to create similarly formatted
Status instances in order to reduce the string literal bloat in
the binary.

What changes are included in this PR?

Helper function UserDefinedLiteralToArrow::FailedToUnpack.
Also added ARROW_PREDICT_FALSE to conditions before calling the
function.

Are these changes tested?

No. Normally there are no tests to verify specific Status messages.
@github-actions github-actions bot mentioned this issue Jan 4, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@westonpace westonpace

Labels
Component: C++ good-first-issue Type: enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@westonpace @benibus