diff mbox series

[FFmpeg-devel,2/4] avcodec/xpm: Minor speed increase for mod_strcspn() {string, reject}==0

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()
Related show

Checks

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

Commit Message

Jose Da Silva Feb. 23, 2021, 4:32 a.m. UTC
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

Comments

Moritz Barsnick Feb. 24, 2021, 1:56 p.m. UTC | #1
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
Jose Da Silva Feb. 26, 2021, 5:34 a.m. UTC | #2
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 mbox series

Patch

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;
         }
     }