librc/run/shutdown: precalc strlen moved out of bounds loop#790
librc/run/shutdown: precalc strlen moved out of bounds loop#790GermanAizek wants to merge 1 commit intoOpenRC:masterfrom
Conversation
|
Note that there's at least one sight-impaired contributor to this repository who will be looking at this PR. Please include godbolt links as well as screenshots to make it accessible. |
|
Technically, this should be fine; assuming that those strings don't change inside the loop (didn't verify myself). But my gut feeling is that it falls into intangible micro-optimization. If we wanted to be serious about string performance, we'd rid of nul-strings entirely. (Not that I advocate doing it, as it'd be a pretty big and error prone rewrite for little gain). |
|
the assembly shows, the call to strlen happens before label that is used for looping-- so in a way, the compiled code is already acting like this is merged merging this is still good imo because it's clearer, and doesn't rely on compiler optimization to make the code not be awful |
Is it? It de-syncs the string pointer and the string length. And so if someone in the future edits it so that the pointer changes during the loop but then forgets to change the length then bad stuff happens. |
| @@ -320,9 +323,9 @@ write_prefix(const char *buffer, size_t bytes, bool *prefixed) | |||
| } | |||
|
|
|||
| if (!*prefixed) { | |||
There was a problem hiding this comment.
this codepath is called once per-line. while calling this function with multiline strings is certainly valid, and i'm sure done in places, it's not the common case, and I have a hard time seeing strlen being called on tiny strings showing up in any perf situation.
len_ec & ec_normal will be like 7 bytes each. len_prefix will be ~length of longest service name, and that's likely to also be quite small.
| while ((utmp = getutxent()) != NULL) { | ||
| if (utmp->ut_type != USER_PROCESS || utmp->ut_user[0] == 0) | ||
| continue; | ||
| if (strncmp(utmp->ut_line, _PATH_DEV, strlen(_PATH_DEV)) == 0) |
There was a problem hiding this comment.
_PATH_DEV is a constant string. if the compiler can't optimize that to a constant integer at compile time, it's not worth caring about. drop this change please.
| } | ||
| free(buffer); | ||
| if (pid == 0 && strlen(my_ns) && strlen (proc_ns) && strcmp(my_ns, proc_ns)) | ||
| if (pid == 0 && len_my_ns && strlen (proc_ns) && strcmp(my_ns, proc_ns)) |
There was a problem hiding this comment.
these strlen calls simply want to know if the string is empty. that can be checked by my_ns[0] and proc_ns[0] -- no need for strlen at all.
5 participants
@navi-desu,
Example with clang-19 -O2
Before:
After: