0

Consider this piece of code

| 34 static bool                                                                     
| 35 _valid_character(char *str, size_t *idx)                                        
| 36 {                                                                               
| 37   char c = str[*idx];                                                           
| 38                                                                                 
| 39   if (c != '\\' && c != '"') {                                                  
| 40     (*idx) += 1;                                                                
| 41     return true;                                                                
| 42   } else if (c == '"') {                                                        
| 43     return false;                                                               
| 44   } else {                                                                      
| 45     char b = str[(*idx) + 1];                                                   
| 46     switch (b) {                                                                
| 47       case '"':                                                                 
| 48       case '\\':                                                                
| 49       case '/':                                                                 
| 50       case 'b':                                                                 
| 51       case 'f':                                                                 
| 52       case 'n':                                                                 
| 53       case 'r':                                                                 
| 54       case 't':                                                                 
| 55         (*idx) += 2;                                                            
| 56         return true;                                                            
| 57       default:                                                                  
| 58         pprint_error("%s@%s:%d invalid escape sequnce \\%c%c (aborting)",       
| 59             __FILE_NAME__, __func__, __LINE__, c, b);                           
| 60         abort();                                                                
| 61     }                                                                           
| 62   }                                                                             
| 63 }  

This one function is the root cause of a meaning full slowdown in my code. I have tried only using if statements and only using switch statements but this is the best optimization that I can come up with where callgrind results in the best performance. This function accounts for about 25% of the runtime so by the questionable (sorry) 80/20 rule its in my best interest to make this code faster.

Below is the callgrind output visualized with kcachegrind on this function.

enter image description here

It seems callgrind is saying that my first jump is the worst jump but I have tried all combinations of this if statement to try to minimize jumping and every time the first jump is the worst jump.

This code was compiled with clang

clang ... -Weverything -Werror -Wpedantic -m64 -O0 -g

So my question is what is the best way to go about optimizing this code and alternative techniques including assembly modification to optimize this simple yet deadly piece of code.

I would like to keep using -O0 since I find it the most useful for debugging and finding optimizations. -O1,2,3,fast tend to abstract to much away to get a good understanding of what is happening.

-- Edit 1 It was asked for an example input.

char cstr[BUF] = "abcdefghijklmnopqrstuvwxyz\"randomgarbageafterthis";
size_t idx = 0;
while (_valid_character(cstr, &idx));

In the end the input is just a string and a loop is called until the end " character. The ending idx value holds cstr[idx] == '"'' to be true.

washcloth
  • 2,730
  • 18
  • 29
  • 2
    Would you give us please a complete example with example inputs and your types? (So we can reproduce your performance and all have the same set of input) – JCWasmx86 Aug 27 '20 at 07:26
  • @JCWasmx86 an example usage of this was appended to the question. – washcloth Aug 27 '20 at 07:33
  • 2
    I'm afraid, this is quite too few of data for a benchmark, one string isn't that much. Furthermore, you should do benchmarks always with the highest optimization level, as `-O0` doesn't mirror the performance, as there are loads and stores for every line. – JCWasmx86 Aug 27 '20 at 07:39
  • 2
    Asking for good performance and using `-O0` is merely exclusive and does not make much sense. This mode basically compiles as specified for the abstract machine. You don't give the compiler a chance to show their potential. – Jens Gustedt Aug 27 '20 at 07:54
  • Instead of checking if every character is valid, why not search for the first (possible) invalid character? – stderr Aug 27 '20 at 08:11
  • You possibly should add code for the `\377` and `\0xFF` escapes, too. – wildplasser Aug 27 '20 at 12:08

1 Answers1

0

If you really want to optimize your code, use pen and paper and create a boolean logic table and optimize its algebra with your own mind. After that recreate your code and please use break; within your switch statements.

For example:

This is your code.

if (c != '\\' && c != '"')
{
    (*idx) += 1;
    return true;
}

This one is faster than the above one.

if (c != '\\')
{
    if (c != '"')
    {
        (*idx) += 1;
        return true;
    }
}

Instead of comparing to some chars like if (c != '\\') use registered CONSTANTS.

register const char BACKSLASH = '\\';

And also do:

register char c = str[*idx];

Constants and variables which are loaded in CPU register are way faster in comparing.

if (c != BACKSLASH)

Generally, if you want fast code, only use if and else and use simple boolean compare arguments.

EDIT:

Code sample 1:

if (c != '\\')
{
    if (c == '"')
    {
        return false;
    }
    else
    {
        (*idx) += 1;
        return true;
    }
}

is equivalent to:

Code sample 2:

if (c != '\\')
{
    if (c != '"')
    {
        (*idx) += 1;
        return true;
    }
}
else if (c == '"')
{
    return false;
}

But the first code example only compares 2 times, but the second code compares 3 times.

paladin
  • 765
  • 6
  • 13
  • 1
    Could you explain why these changes should improve performance? Given that C uses short circuit evaluation for conditions, there should not be a measurable difference. – Gerhardh Aug 27 '20 at 09:36
  • What do you mean with "use break"? Should they use `break` instead of `return` or should only single value cases be used and the instruction copied to each single case? – Gerhardh Aug 27 '20 at 09:37
  • Short circuit evaluation only works for simple arguments. Here an example were compiler short circuit evaluation doesn't work: `if (0 && i++){}`. Yes the instruction should be copied for every case in combination with `break;`, otherwise the `switch` statement compares every case, even if it's already true. – paladin Aug 27 '20 at 10:57
  • 1
    How does it not work? It works perfectly fine. `i++` will never be executed exactly same as with `if(0){if(i++){}}` – Gerhardh Aug 27 '20 at 11:06
  • Do you have any reference that `switch` would compare any case values after a matching is found? – Gerhardh Aug 27 '20 at 11:06
  • i++ will be executed, even if the first && argument is already false. That's why the short circuit evaluation doesn't work for every case, that's why I'm suggesting the coding way above. I'm sorry, I was wrong with switch, I've mistaken the all case execution with an all compare. Switch compares only as long as its not true. In my opinion using breaks; with switch is a cleaner way to program, especially if you want to program hard and fast boolean algebra. – paladin Aug 27 '20 at 11:35
  • 1
    If the first part of the expression is already `false`, the part after `&&` will not be evaluated because the result can never get `true` any more. That is what short circuit evaluation does. – Gerhardh Aug 27 '20 at 11:37
  • You are right, I tested it, this is curios, because that means: `int i = 0; int j = 0;` -> `if (0 && i++)` != `if (j++ && 0)` – paladin Aug 27 '20 at 12:13
  • 1
    Correct. That is the main purpose of short circuit evaluation. – Gerhardh Aug 27 '20 at 12:14