search

zeek / spicy

C++ parser generator for dissecting protocols & files.
https://docs.zeek.org/projects/spicy
Other
243 stars 37 forks source link

Prevent defining already forwarded constants. #1786

Closed bbannier closed 1 month ago

bbannier commented 1 month ago

Closes #1785.

bbannier commented 1 month ago

I checked this as part of Zeek for compiling a giant internal analyzer which took 2x the time before this patch. With this PR compilation time is down to roughly the previous level again.

I still need to check whether the way the compiler cannot inline anymore (the linker could though) has any negative performance impact with Spicy-based Zeek analyzers.

For our huge internal analyzer runtime perf seems to be unaffected (compiled with clang on macos-13.6.7):

$ hyperfine -w 3 -L P BEFORE,AFTER 'zeek {P}/*hlto ../scripts -Cr xyz.pcap'
Benchmark 1: zeek BEFORE/*hlto ../scripts -Cr xyz.pcap
  Time (mean ± σ):      1.897 s ±  0.035 s    [User: 1.810 s, System: 0.191 s]
  Range (min … max):    1.859 s …  1.949 s    10 runs

Benchmark 2: zeek AFTER/*hlto ../scripts -Cr xyz.pcap
  Time (mean ± σ):      1.868 s ±  0.025 s    [User: 1.780 s, System: 0.192 s]
  Range (min … max):    1.840 s …  1.910 s    10 runs

Summary
  zeek AFTER/*hlto ../scripts -Cr xyz.pcap ran
    1.02 ± 0.02 times faster than zeek BEFORE/*hlto ../scripts -Cr xyz.pcap
rsmmr commented 1 month ago

I still need to check whether the way the compiler cannot inline anymore (the linker could though) has any negative performance impact with Spicy-based Zeek analyzers.

Forgot to comment on this: I think it's fine. I believe we create constant forwards only for TypeInfos, so that's the only place where things should change (and for them, inlining doesn't matter).