Closed tocic closed 1 year ago
Thanks for the suggestion! I have run the benchmark with these modified structs; here's what I got:
Results for Debug builds:
| **Debug** | _snitch_ | _Catch2_ | _doctest_ | _Boost UT_ |
|-----------------|----------|----------|-----------|------------|
| Build framework | 2.0s | 41s | 2.0s | 0s |
| Build tests | 62s | 79s | 73s | 118s |
| Build all | 64s | 120s | 75s | 118s |
| Run tests | 37ms | 76ms | 63ms | 20ms |
| Library size | 3.6MB | 38.6MB | 2.8MB | 0MB |
| Executable size | 31.6MB | 49.3MB | 38.6MB | 51.9MB |
Results for Release builds:
| **Release** | _snitch_ | _Catch2_ | _doctest_ | _Boost UT_ |
|-----------------|----------|----------|-----------|------------|
| Build framework | 2.8s | 47s | 3.5s | 0s |
| Build tests | 127s | 254s | 207s | 289s |
| Build all | 130s | 301s | 210s | 289s |
| Run tests | 25ms | 46ms | 44ms | 5ms |
| Library size | 0.68MB | 2.6MB | 0.39MB | 0MB |
| Executable size | 8.3MB | 17.4MB | 15.2MB | 11.3MB |
The run time increased slightly in debug and decreased slightly in release; it's only a few milliseconds though, that could be just noise. The total build time is unchanged. The library size shrunk a little bit in debug, but stayed mostly the same in release.
Overall, not much difference. But it's essentially free, so why not?
The only concerns I have:
assertion_location
struct), and b) regardless, if the grouping is important for comprehension, this should be reflected in the documentation, not in the code. So I think this isn't a good argument against the change. It also doesn't apply here; there is no grouping being broken.duration
member as last member in the struct on purpose, so that the other elements of the structs are read at the same address, regardless of the SNITCH_WITH_TIMINGS
macro. I thought this was a good idea in case someone messed up their build, with some files having the SNITCH_WITH_TIMINGS
macro and others not: there would be no observable difference unless duration
was used explicitly, and then it's a compilation error. It's kind of a niche use case though. And perhaps also naive; having some files compiled with SNITCH_WITH_TIMINGS
and some files without is undefined behavior regardless. So :shrug:. I'm not too attached to that one as well.If you also think the above doesn't contain a justifiable reason not to make the change you suggest, then please open a PR ;)
Yep, I agree with your reasoning, but maybe the third point is actually a minus for the users since they can't easily spot the ODR violation (an example). Also, there's the Hyrum's law to consider here, but I believe it's not much a concern either.
Looks like we can reduce the size of these structs by rearranging the members according to their sizes in decreasing order:
https://github.com/cschreib/snitch/blob/5ad2fffebf31f3e6d56c2c0ab27bc45d01da2f05/include/snitch/snitch_test_data.hpp#L41-L51
https://github.com/cschreib/snitch/blob/5ad2fffebf31f3e6d56c2c0ab27bc45d01da2f05/include/snitch/snitch_test_data.hpp#L57-L64
See the comparison.
It could potentially improve the benchmark too.