Get column value by column label in Framework Core ASoA #13498
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
Thank you for your investigation. Much appreciated. As a general note, please try to have a smaller contributions if possible, because that will for sure simplify and speedup the review process. |
Please consider the following formatting changes to AliceO2Group#13498
|
Previously, I used I am currently stuck with this solution; I would like to know if using the get() explicitly column struct member method is possible. I was trying to implement such an approach, but there is a type problem; the member method pointer type is ex. I would like to know if you think such a performance difference would be acceptable and if you can think of a more efficient solution. |
|
@mytkom thank you for your work. See inline comments. I might have more once those are resolved. |
|
@ktf |
Please consider the following formatting changes to AliceO2Group#13498
|
@ktf |
Please consider the following formatting changes to AliceO2Group#13498
|
Error while checking build/O2/fullCI for 6e55f8d at 2024-10-21 18:16: Full log here. |
|
Good morning @ktf, |
|
@ktf |
| // allows user to use consistent formatting (with prefix) of all column labels | ||
| // by default there isn't 'f' prefix for dynamic column labels | ||
| bool isPrefixMatch = columnLabel.size() > 1 && columnLabel.substr(1) == C::columnLabel(); | ||
| // check also exact match if user is aware of prefix missing | ||
| bool isExactMatch = columnLabel == C::columnLabel(); | ||
|
|
||
| return (isPrefixMatch || isExactMatch) ? &getColumnValue<R, T, C> : nullptr; |
There was a problem hiding this comment.
Nevermind the previous post, which I accidentally deleted. You can still return immediately, no? Is it already optimised out? I do not think the compiler is allowed to do that.
if (columnLabel.size() > 1 && columnLabel.substr(1) == C::columnLabel()) {
return &getColumnValue<R, T, C>;
}
if (columnLabel == C::columnLabel()) {
return &getColumnValue<R, T, C>;
}
return nullptr;There was a problem hiding this comment.
You're right. It would be faster to return immediately.
What do you think is not allowed by the compiler? Comparison of const char* and string_view?
|
The template <typename R, typename T, persistent_with_common_getter<R> C>
ColumnGetterFunction<R, T> createGetterPtr(const std::string_view& targetColumnLabel)
{
return targetColumnLabel == C::columnLabel() ? &getColumnValue<R, T, C> : nullptr;
}
template <typename R, typename T, dynamic_with_common_getter<R> C>
ColumnGetterFunction<R, T> createGetterPtr(const std::string_view& targetColumnLabel)
{
std::string_view columnLabel(C::columnLabel());
// allows user to use consistent formatting (with prefix) of all column labels
// by default there isn't 'f' prefix for dynamic column labels
if (targetColumnLabel.starts_with("f") && targetColumnLabel.substr(1) == columnLabel) {
return &getColumnValue<R, T, C>;
}
// check also exact match if user is aware of prefix missing
if (targetColumnLabel == columnLabel) {
return &getColumnValue<R, T, C>;
}
return nullptr;
} |
I will continue our discussion here @ktf, @jgrosseo, @saganatt.
@jgrosseo was correct; this solution shouldn't be implemented as it is. It was slower in this benchmark by 3-10 times for simple examples. I will leave this draft PR here and try to implement this alternative solution I was writing about in AliceO2Group/O2Physics#7371. That 20% I was speaking of during the presentation was the whole task performance difference locally on my laptop. These are my local benchmark results: