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

Does it make sense to optimize strlen in this function with for loops? #7268

Open
GermanAizek opened this issue May 13, 2024 · 4 comments
Open

Comments

@GermanAizek
Copy link
Contributor

Function ggml_graph_dump_dot use 2 strlen() in ggml.c
Function ggml_visit_parents use 2 strlen() in ggml.c
Function ggml_cpy_impl use 1 strlen() in ggml.c

Example:

Before:

image

After:

image

@ngxson
Copy link
Collaborator

ngxson commented May 14, 2024

ggml_graph_dump_dot only used for debugging, so it is not important.

ggml_visit_parents is called by ggml_build_forward_expand, which is called every time llama_decode is called. Probably OK to remove strlen here, but performance gain will be very, very minor.

ggml_cpy_impl is not called very often, not important to optimized.

IMO, I think it's really not worth doing this. Writing strlen(...) == 0 is still more self explanatory (easier for new contributors to read the code)

@GermanAizek
Copy link
Contributor Author

@ngxson, I can only fix for ggml_visit_parents, but in comments I specify previous condition with strlen function so that it is readable.

@ngxson
Copy link
Collaborator

ngxson commented May 14, 2024

If you want, another idea would be to add a macro IS_STRING_NOT_EMPTY / IS_STRING_EMPTY and re-use it through out the code base

@slaren
Copy link
Collaborator

slaren commented May 14, 2024

We do not want to apply optimizations that serve no real purpose but make the code harder to read. And anyway, the compiler is perfectly capable of optimizing a strlen(s) == 0, eg. https://godbolt.org/z/ocPEv39b7

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

No branches or pull requests

3 participants