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

[compute] Apply std::string_view #14686

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ragmani
Copy link
Contributor

@ragmani ragmani commented Feb 17, 2025

This commit applies std::string_view to CLKernelLibraryEx and Einsum.

ONE-DCO-1.0-Signed-off-by: ragmani [email protected]

This commit applies std::string_view to CLKernelLibraryEx and Einsum.

ONE-DCO-1.0-Signed-off-by: ragmani <[email protected]>
void set_kernel_path(const std::string &kernel_path) { _kernel_path = kernel_path; };
void set_kernel_path(std::string_view kernel_path) { _kernel_path = kernel_path; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can use const std::string_view as a parameter. However, since std::string_view is a lightweight, non-owning value type that is typically passed by value, adding const doesn't provide extra safety or performance benefits in most cases. It simply prevents modifications to the local copy of the view inside the function, which is often unnecessary.

mutable std::map<std::string, const Program>
mutable std::map<std::string, const Program, std::less<>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/utility/functional/less_void

A transparent comparator is a comparator type that supports heterogeneous comparisons—that is, it allows you to compare two objects even if they are of different types, as long as they are comparable. In C++ standard library associative containers (like std::map), a comparator is considered transparent if it defines a nested type called is_transparent. For example, std::less<> is a transparent comparator, which lets you compare an std::string with an std::string_view without having to convert one to the other.

result.push_back(text.substr(start, pos - start));
result.push_back(std::string(text.substr(start, pos - start)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using std::string_view internally can reduce unnecessary copying during the splitting process, which might improve performance in some scenarios. However, since each substring is eventually converted to an std::string for the returned vector, the benefit might be less pronounced if most of the work is spent on that conversion. In many cases, the performance difference compared to the original code will be minor, but for large input strings or in performance-critical contexts, avoiding extra copies could provide some speed improvements.

@ragmani ragmani requested a review from a team February 17, 2025 10:08
@ragmani ragmani added the PR/ready for review It is ready to review. Please review it. label Feb 17, 2025
seockho-kim
seockho-kim previously approved these changes Feb 18, 2025
Copy link
Contributor

@seockho-kim seockho-kim left a comment

Choose a reason for hiding this comment

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

LGTM

ys44kim
ys44kim previously approved these changes Feb 18, 2025
Copy link
Contributor

@ys44kim ys44kim left a comment

Choose a reason for hiding this comment

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

LGTM

@ragmani ragmani dismissed stale reviews from ys44kim and seockho-kim via bf1ba45 February 18, 2025 09:29
Comment on lines 261 to 262
void CLKernelLibraryEx::add_built_program(std::string_view built_program_name, cl::Program program)
void CLKernelLibraryEx::add_built_program(const std::string &built_program_name,
cl::Program program)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since _built_programs_map is returned via a member function as const std::map<std::string, cl::Program>&, I couldn't use a transparent comparator. Therefore, I reverted the use of std::string_view here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants