| Message ID | 202102222032.14091.digital@joescat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/xpm: Minor speed increase to function hex_char_to_number() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Mon, Feb 22, 2021 at 20:32:14 -0800, Jose Da Silva wrote: > - for (i = 0; string && string[i]; i++) { > + if (string == 0) "if (!string)" is the preferred style for pointers. But I don't see the advantage - isn't the loop interrupted immediately anyway if string == NULL? (Same for the other loop you changed.) Inside the loops, the additional checks for validity of "string" seem redundant either way, though: while ( string && string[i] && string[i] != '\n' ) Moritz
On February 24, 2021 05:56:17 AM Moritz Barsnick wrote: > On Mon, Feb 22, 2021 at 20:32:14 -0800, Jose Da Silva wrote: > > - for (i = 0; string && string[i]; i++) { > > + if (string == 0) > > "if (!string)" is the preferred style for pointers. Works for me. Updated patches reworked for today's date. > But I don't see the advantage - isn't the loop interrupted immediately > anyway if string == NULL? (Same for the other loop you changed.) Yes, ...but... let's now look inside the for loop with string != NULL If we read 100 character before reaching reject, that is 100 extra tests. Technically speaking, we could remove this test since the function is private to xpmdec.c, but if the function is useful enough to be globally used (instead of private here), it is worth having the extra check as a safety-check. one_test vs no_test is not a crushing impact on speed, but it is still better than loopin and testing "i" times. The other loop is inside the first loop, so this effect is multiplied. If we attempt to reject, say... ",}" then we test "reject!=0" three times If we need to search through ten characters in string, that makes it about thirty tests. Take this outside the for loop, and that is now ten tests. It is a minor speed improvement, and I thought it was worth resolving these low hanging fruit first. I wanted to avoid too much churn which is likely when submitting a larger set of patches :-) In terms of churn - Hopefully the "if (!string)" was the only catch here. > Inside the loops, the additional checks for validity of "string" > seem redundant either way, though: > > while ( string && string[i] && string[i] != '\n' ) ...well, if we are already paying attention to string&reject in the outside loops, we might as well go ahead and fix the same/similar things here too. These are also minor speed improvements, and two fewer if/then/else in the compiled binary sense too. Joe
diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c index 5aab46c52d..92a568041c 100644 --- a/libavcodec/xpmdec.c +++ b/libavcodec/xpmdec.c @@ -213,22 +213,24 @@ static size_t mod_strcspn(const char *string, const char *reject) { int i, j; - for (i = 0; string && string[i]; i++) { + if (string == 0) + return 0; + for (i = 0; string[i]; i++) { if (string[i] == '/' && string[i+1] == '*') { i += 2; - while ( string && string[i] && (string[i] != '*' || string[i+1] != '/') ) + while (string[i] && (string[i] != '*' || string[i+1] != '/')) i++; i++; } else if (string[i] == '/' && string[i+1] == '/') { i += 2; - while ( string && string[i] && string[i] != '\n' ) + while (string[i] && string[i] != '\n') i++; - } else { - for (j = 0; reject && reject[j]; j++) { + } else if (reject) { + for (j = 0; reject[j]; j++) { if (string[i] == reject[j]) break; } - if (reject && reject[j]) + if (reject[j]) break; } }
Test string==0 once before looping. Avoid testing inside the loop. Test reject==0 once before looping. Avoid testing inside the loop. Signed-off-by: Jose Da Silva <digital@joescat.com> --- libavcodec/xpmdec.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) -- 2.30.1