Build time improvements

[Klaim]

This set of changes attempts to reduce the total build-time of ArcticDB C++ code.

After many experiments I ended up going with the following strategy:

• reduce the inclusion of protobuf headers in headers which are used everywhere (notably types.hpp)
• reduce inclusions in general (still a lot of not-used includes)
• reduce code appearing in headers by moving the implementations in translation units where it seems trivial and harmless

[Klaim]

At this moment:

• the release build doesnt work for me locally, seems like a dependency handling issue
• the feedback I get from Visual Studio's Build Insights points to a drastic reduction of the percentage of time spent on types.hpp
• I started but didnt finish and didnt included here work on memory_segment.hpp which took the place of header the most expensive overall in the build times (probably because it again forces inclusion of protobuf)
• Some tests are not passing, I suspect this is due to a change I had to make to avoid conversions from types to protobuf types, I didnt find explicit usage of that code but maybe this is silently changing the meaning of the serialization code.

Note also that now types.hpp have a types_proto.hpp including it plus the code specific to protobuf. Same thing for other headers I'm in the process of modifying.

In the future, it would be good if the code was not merging functionality and protobuf data/reflection. It usually can be done through functions/types knowing both the non-protobuf and the protobuf data types and doing the bridge work.

[vasil-pashov]

We're moving things around anyways, why don't we move these functions into their own cpp?

[Klaim]

What do you call "these functions" exactly?
I did plan a step where I move inlined functions in cpps, although it took me a while just to make this thing build XD
If you meant the protobuf-related functions, the issue is that a lot of these functions are member functions, so it's not trivial work to just move them, but yes I think I'll be forced to.

[Klaim]

Oh ok you pointed the whole header, I didnt realize as it was not clear in the github discussion.
I'll try to move what I can in the cpp although now this header is not costing too much in the build time at the moment so I decided to do that after I'm done with memory_segment.hpp. TLDR: wait for it 👍🏽

[Klaim]

In incoming commits I moved the functions which could be in the cpp, but IndexDescriptor cannot be moved in there unfortunately, it's used in a lot of other files. It's also explciitly exposing protobuf types so as long as it's the case it's impossible to remove that include to protobuf types :/

[Klaim]

I had to remove most of my changes in memory_segment and related headers because it's too intertwined with templates and protobuf.
I started a pass of moving inline code into cpps but am not finished yet.

[vasil-pashov]

the release build doesnt work for me locally, seems like a dependency handling issue

Snappy is used in rapidcheck. You can either unload rapidcheck project or go and manually set MTd flags to project related to that.

[Klaim]

I still have more changes ongoing but I think to avoid future conflicts this could be merged. So I guess it's time to review 👍🏽 my other changes relies on these ones.

[alexowens90]

@Klaim can you post some data on the wall-clock time improvements? Both locally and on the CI? I'm running some little tests, but would be good to know what you've found too.

[Klaim]

@Klaim can you post some data on the wall-clock time improvements? Both locally and on the CI? I'm running some little tests, but would be good to know what you've found too.

On the CI unfortunately I'm dependent on master not failing, but otherwise ok I'll setup a comparison.
I'll try to make some measurements tomorrow, although I noticed it varies a lot on my machine. The main information I can give which seems constant is that types.hpp doenst take anymore 9% of the build time but more like 3%. However that doesnt necessarilly translates to shorten build time, so something weird is going on, I've been searching for what's happening for a few days.

[Klaim]

@alexowens90 The CI is too unreliable to make comparisons unfortunately so not sure what to do about that.

I made some local build-insights records yesterday to get a better picture, here is the raw data (you can open it on windows in either Visual Studio or using Windows Performance Analyzer, I focused on the VS part below): https://drive.google.com/file/d/1Vo_TgO9mNRYaq4AOKwwR4s7p-ntO4HDL/view?usp=sharing

This is captured on a machine using Windows 11, VS 17.9.0-preview5, vcpkg, cmake preset "windows-vs-2022", 6 jobs set in cmake but the machine has 24 logical cores, AMD Ryzen 9 5900X 12 real cores and 32GO of RAM. I stopped using the computer once the builds were launched although some other programs where running.
Note that I used 6 jobs to avoid saturating the memory.
Anyway I wouldnt rely on that as hard data as it is not a completely isolated and fixed system and the number of builds are not big enough for relevant stats. But its still a general idea of the experience of building arcticdb on windows I guess.

Here is a summary:

Sorted by build times:

Here is one of the samples from master in Release when open in VS, see the annotation about types.hpp which is one of the files I focused on in this PR:

Here is another sample from this PR branch in Release, see the annotation for the same file:

My interpretation:

• Debug builds buildtimes varies too much to deduce anything. Once hot (as in the build is repeated several times) the build seems to take around 327secs most of the time on this branch, and sometimes (once here) on master. This is not conclusive.
• Release builds tends to be more stable in duration and we can see there is about 10-13 secs of improvement for this 6-jobs scenario. Not sure how it would be different for 1job (is the ci still using 1j on Windows?) or 24jobs here (needs a ram upgrade first). In any way while I see improvements, these are less than I expected or even measured when starting the work here.
• The types.hpp change from around 9% of the build time to 3% is a big win but it doesnt affect too much the overall build time. I'm not sure why exactly but I didnt have the time to inspect in detail every measurements using Windows Performance Analyzer. Same for some other headers I tried to reduce the impact of.
• There are changes I had locally that I had to scrap because they would change too much of the existing code, so I cut these for now as these changes here would be a fundation before adding the other ones. Not sure it's worth the time spent on it though.

While doing this and thinking about the discussions about C++20 one thing I kept thinking is that it might be far more worth our time to just make a modularized version of arcticdb libraries, as that doesnt change anything to the Python API but would drastically reduce the build time with an effort that's far easier to setup (but still an effort). Maybe that would be worth exploring once arcticdb switches to C++20.

[jamesmunro]

Thanks for all the work here @Klaim Please merge when ready.