bitdefender / bddisasm

bddisasm is a fast, lightweight, x86/x64 instruction decoder. The project also features a fast, basic, x86/x64 instruction emulator, designed specifically to detect shellcode-like behavior.
Apache License 2.0
888 stars 115 forks source link

Formatting improvements #93

Closed turol closed 3 months ago

turol commented 3 months ago
  1. Add a macro to reduce code duplication
  2. Make some arrays more const. This causes no code changes but is slightly cleaner. In theory the compiler could optimize more or put those arrays in read-only data.

I originally tried to make it faster by reducing unnecessary processing in nd_strcat_s. It has to skip over the beginning of the string which takes quite a bit of time. That code is not part of this PR but is here: https://github.com/turol/bddisasm/commits/strcat_opt/ https://github.com/turol/bddisasm/commit/84aa9801cc724334f619c7f09d09bb354c5942b7

The problem is that it requires either changing the return value of nd_strcat_s (breaking other people's integrations) or adding a new variant which only differs in return value. Would the second one be acceptable?

vlutas commented 3 months ago

Hello & thank you for your contributions!

  1. The macro to reduce code duplication is OK
  2. Making the arrays more const won't help that much (performance wise it likely won't matter); However, there is no harm either, so this is OK as well

As for refactoring of the nd_strcat_s - the formatting function is not meant to be performant as it is, since it will be quite slow (lots of ifs and string concatenation...). In any way, we don't change functionality that may affect integrators, and we certainly don't modify existing functions in such a way (for example, changing the return value). Creating a different nd_strcat_s function would be fine, but as I said - I see no reason to insist on this, as performance gain would probably not be very high. But if you have some actual performance figures, to see how much it improves formatting, that would be nice.

Thanks!

turol commented 3 months ago

Do you have on opinion on what the new nd_strcat_s function variant should be named?

Before:

2,473.09        msec task-clock
11,314,302,881  cpu_core/cycles/

After:

2,453.29        msec task-clock
11,224,008,441  cpu_core/cycles/

So about 1% speedup. I think further gains are possible but it's going to be more complicated. This was a relatively simple change.

vlutas commented 3 months ago

For 1% speedup, I don't think it's worth doing that change right now in bddisasm, especially since that code is not performance critical at all. Performance-wise, we want bddisasm to be fastest when decoding the instructions, not necessarily when formatting them to a string; when working with instructions, most use-cases don't even require the textual disassembly to be generated, unless it's for logging/debugging purposes.