-
Notifications
You must be signed in to change notification settings - Fork 915
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
Fix ingest_raw_data performance issue in Nested JSON reader due to RVO #12070
Fix ingest_raw_data performance issue in Nested JSON reader due to RVO #12070
Conversation
if (compression == compression_type::NONE) { | ||
return buffer; | ||
} else { | ||
return decompress(compression, buffer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I also didn't know that ternary expression could prevent copy elision
Ref: https://stackoverflow.com/questions/22078029/why-does-the-ternary-operator-prevent-return-value-optimization
If possible, can you make a simple example in goldbolt (class printing to stdout when constructed/copied) to demonstrate that, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://godbolt.org/z/1vzPqqzs4
Here is a sample code.
tertiary operator calls vector(vector const&)
for buffer return
but if() return buffer;
calls vector(vector&&)
.
Reason for this could be - for tertiary operator, one is lvalue (buffer
), and the other is rvalue
(return value of another function).
So, for lvalue, it makes call to copy constructor.
For Rvalue, it called move constructor.
Codecov ReportBase: 87.47% // Head: 88.12% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #12070 +/- ##
================================================
+ Coverage 87.47% 88.12% +0.64%
================================================
Files 133 135 +2
Lines 21826 22011 +185
================================================
+ Hits 19093 19397 +304
+ Misses 2733 2614 -119
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a brilliant catch.
@gpucibot merge |
Description
Issue is that
json::experimental::ingest_raw_data
took double the time ofjson::ingest_raw_data
for same data.After replacing tertiary operator with
if
else
, runtime for 500 MB file is same asjson::ingest_raw_data
I suspect, RVO (copy elision) is skipped while using tertiary operator.
Checklist