ggerganov/llama.cpp

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

GermanAizek opened this issue · 4 comments

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

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)

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

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

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