uptane / aktualizr

C++ Uptane Client
Mozilla Public License 2.0
16 stars 16 forks source link

Tidy up HttpFake #96

Closed cajun-rat closed 1 year ago

cajun-rat commented 1 year ago

I've been looking into a problem with targets that contain non-ascii characters. While doing the investigation work, I tidied up a few places, which formed this PR.

For background, Aktualizr and libtuf have different ideas about how to canonicalise JSON. The details are in my multibyte branch, with this commit being the critical one that would change things.

For now, I hope these changes are benign and will be useful in general.

cajun-rat commented 1 year ago

Were the header changes done just to speed up the tests?

Mostly it is because I'm allergic to implementations in header files, and the include file bloat that it brings. I did some quick tests by setting CCACHE:BOOL=OFF in CMakeCache and running ninja clean && perf stat ninja -j 4 build_tests, and the results were:

Old

 Performance counter stats for 'ninja -j 4 build_tests':

        933,205.02 msec task-clock:u              #    3.925 CPUs utilized          
                 0      context-switches:u        #    0.000 /sec                   
                 0      cpu-migrations:u          #    0.000 /sec                   
        16,649,407      page-faults:u             #   17.841 K/sec                  
 2,035,613,701,466      cycles:u                  #    2.181 GHz                    
 2,579,815,611,966      instructions:u            #    1.27  insn per cycle         
   555,977,790,186      branches:u                #  595.772 M/sec                  
    15,213,921,219      branch-misses:u           #    2.74% of all branches        

     237.746429511 seconds time elapsed

     860.779101000 seconds user
      72.533818000 seconds sys

New

 Performance counter stats for 'ninja -j 4 build_tests':

        904,339.02 msec task-clock:u              #    3.830 CPUs utilized          
                 0      context-switches:u        #    0.000 /sec                   
                 0      cpu-migrations:u          #    0.000 /sec                   
        16,258,966      page-faults:u             #   17.979 K/sec                  
 1,974,390,234,132      cycles:u                  #    2.183 GHz                    
 2,508,911,757,785      instructions:u            #    1.27  insn per cycle         
   540,618,156,626      branches:u                #  597.805 M/sec                  
    14,820,741,421      branch-misses:u           #    2.74% of all branches        

     236.115153080 seconds time elapsed

     832.612446000 seconds user
      72.257427000 seconds sys

So it is maybe a tiny bit faster. There is a loss because the libtestutilities.a static library that gets linked into every test is quite a bit bigger. It seems like we should be able to make that a shared library, but when I tried that I hit linker errors that I haven't got to the bottom of yet.

pattivacek commented 1 year ago

Were the header changes done just to speed up the tests?

Mostly it is because I'm allergic to implementations in header files, and the include file bloat that it brings. I did some quick tests by setting CCACHE:BOOL=OFF in CMakeCache and running ninja clean && perf stat ninja -j 4 build_tests, and the results were:

Nice, I love numbers! I mean, even if there were no speedup, I have no objection at all to the changes. I'm quite happy to get things out of headers.