No description provided.
Activity
11
athre0z4y agoWhile I see why this is necessary under our current API guarantees, I really can't say that I like it too much. It adds a pretty significant amount of branches to our already way too branchy code. I ran a benchmark and the performance regression is pretty noticeable. :/
Performance impact
Full decode + Formatting
Clang
Before
Benchmark 1: ./bench-zydis-full-fmt 0x14 0x400 0x2460400 /home/ath/devel/disas-bench/input/xul.dll
Time (mean ± σ): 13.963 s ± 0.090 s [User: 13.941 s, System: 0.007 s]
Range (min … max): 13.851 s … 14.122 s 10 runsAfter
Benchmark 1: ./bench-zydis-full-fmt 0x14 0x400 0x2460400 /home/ath/devel/disas-bench/input/xul.dll
Time (mean ± σ): 15.572 s ± 0.034 s [User: 15.549 s, System: 0.007 s]
Range (min … max): 15.528 s … 15.614 s 10 runsRegression
~12%
GCC
Before
Benchmark 1: ./bench-zydis-full-fmt 0x14 0x400 0x2460400 /home/ath/devel/disas-bench/input/xul.dll
Time (mean ± σ): 14.245 s ± 0.086 s [User: 14.221 s, System: 0.007 s]
Range (min … max): 14.094 s … 14.362 s 10 runsAfter
Benchmark 1: ./bench-zydis-full-fmt 0x14 0x400 0x2460400 /home/ath/devel/disas-bench/input/xul.dll
Time (mean ± σ): 15.503 s ± 0.089 s [User: 15.478 s, System: 0.007 s]
Range (min … max): 15.301 s … 15.602 s 10 runsRegression
~9%
flobernd4y agoI ran a benchmark and the performance regression is pretty noticeable. :/
That's not nice. Have you been able to profile it? Probably the new branches in ZydisGetInstructionDefinition might be the issue. I will check on Windows as well.
While I see why this is necessary under our current API guarantees...
This was actually my initial reason to do everything in one single pass when we started. Sadly, each approach has some drawbacks. Let's try to find out what's causing the regression and optimize it as good as we can.
athre0z4y agoHere's a perf diff. Note that -4% doesn't mean that ZydisDecoderDecodeInstruction got faster -- it's percentages that are being compared. In other words: ZydisDecoderDecodeOperands etc got heavier, so the share of cycles spent in ZydisDecoderDecodeInstruction decreased in comparison.
$ perf diff build-master/perf.data build-flo/perf.data
# Event 'cpu_core/cycles:u/'
#
# Baseline Delta Abs Shared Object Symbol
# ........ ......... .................... .....................................
#
52.25% -4.17% bench-zydis-full-fmt [.] ZydisDecoderDecodeInstruction
1.90% +2.76% bench-zydis-full-fmt [.] ZydisDecoderDecodeOperands
1.15% +1.25% bench-zydis-full-fmt [.] ZydisGetInstructionDefinition
4.17% +1.13% libc-2.33.so [.] __memset_avx2_unaligned_erms
21.56% -0.56% bench-zydis-full-fmt [.] ZydisDecodeOperands
0.96% +0.37% bench-zydis-full-fmt [.] ZydisDecodeOperandMemory
1.77% -0.27% bench-zydis-full-fmt [.] ZydisRegisterEncode
4.27% -0.19% bench-zydis-full-fmt [.] main
0.84% -0.11% bench-zydis-full-fmt [.] ZydisGetAccessedFlags
0.65% -0.10% bench-zydis-full-fmt [.] ZydisReadDisplacement
0.60% -0.09% bench-zydis-full-fmt [.] ZydisGetElementInfo
+0.09% bench-zydis-full-fmt [.] ZydisCalcRegisterId
0.68% -0.08% bench-zydis-full-fmt [.] ZydisGetInstructionEncodingInfo
0.43% +0.07% bench-zydis-full-fmt [.] memset@plt
2.05% -0.05% bench-zydis-full-fmt [.] ZydisRegisterGetWidth
0.89% -0.04% bench-zydis-full-fmt [.] ZydisFormatterFormatInstruction
1.69% -0.02% bench-zydis-full-fmt [.] ZydisFormatterFormatInstructionEx
2.76% -0.01% bench-zydis-full-fmt [.] ZydisDecoderTreeGetChildNode
0.76% -0.01% bench-zydis-full-fmt [.] ZydisReadImmediate
0.61% -0.00% bench-zydis-full-fmt [.] ZydisGetOperandDefinitions
+0.00% ld-2.33.so [.] update_usable.constprop.0
0.00% -0.00% ld-2.33.so [.] _dl_start
0.00% -0.00% ld-2.33.so [.] _start
0.00% ld-2.33.so [.] init_cpu_features.constprop.0@athre0z How do we proceed with this PR?
athre0z4y agoWell, I'm not really excited about a 10% perf regression for everyone for what is essentially "make debugging easier". The single commit in this PR does a bunch of different things, some of which are entirely uncontroversial and will likely improve rather then regress performance, e.g. using lookup tables instead of chains of ternary expressions. We should probably start by splitting this one commit into multiple smaller ones and then do partial merges.
As for the argument validation, we'll have to make a decision as to whether we simply define this as undefined behavior and keep performance intact or burn a ton of CPU on validation.
flobernd4y agoJust go with undefined behavior is a bad idea, because it will often lead to invalid memory access or other critical side effects. We don't see these effects on master yet, because our fuzzer does not test this. Imho we have to continue using sanity checks for public APIs.
There must be a way to mitigate the performance issues.
We probably should add a new fuzzer project to test the
ZydisDecoderDecodeOperands()function with randomly crafted inputs (contextandinstruction).