[ML] Decrease the memory used by distribution models

[tveasey]

This makes some type changes to reduce precision of quantities where we don't need that precision. For example, this saves us around 15% of the memory typically used by the count residual distribution model (dropping from 1900 to 1650 bytes).

This may have a small affect on results simply due to slightly different rounding of floating point quantities.

[droberts195]

Is this actually making a difference on any platform? On all the platforms we support int is 32 bits. And I know gcc uses int for enums unless it's forced to do something different.

The C++ standard says that compilers can choose any integral type that can accommodate all the enum values to represent the enum type. But in practice they'll probably favour int as the C standard says the enum values must be int, and it would be foolish for a compiler vendor to introduce an incompatibility in the binary output for the same header file depending on whether it was compiled as C or C++.

So does clang or Visual C++ use a different enum size by default? If not, I think this is just introducing a distraction into the code without changing the end result.

[tveasey]

Ok good point. I'd actually switched them to int8_t originally, but it caused knock on problems for printing them out which created more code churn. I'll double check and revert if it doesn't make a difference.

[hendrikmuhs]

The other problem: Even if you force shrink the datatype like this (I agree int should be 32bit also on 64bit) you might not achieve the desired affect due to padding in the class instances where that is used.

(Do we correctly take padding into account in the memory usage calculations?)

[tveasey]

I also shuffled some variable orders around so we would've benefited from shrinking these. I'm just doing a run now though to verify if we do actually get anything from this, I suspect Dave is right though and we don't.

[tveasey]

Also, we're using sizeof to get the size of these classes in the memory calculation (which include pads).

[droberts195]

(Do we correctly take padding into account in the memory usage calculations?)

We take padding between members of a single object into account, because we use sizeof(obj) rather than sizeof(member1) + sizeof(member2) + ....

What we don't take into account is allocator overhead. For example, if we ask for a 27 byte chunk of memory and the OS gives us a 32 byte chunk instead then we'll count 27 if sizeof(obj) is 27. And nor do we take into account any allocator data structures like pointers to the next block in a free list.

But model memory is only meant to be a rough estimate - getting it absolutely perfect would be incredibly hard and would involve having a very deep knowledge of what malloc() is doing on each platform.

[hendrikmuhs]

👍

Maybe there are data structures which would benefit from disabling padding? If so we could try and see what are the savings in memory and what is the added runtime (hopefully not that bad).

[tveasey]

That's an interesting suggestion. I reordered variables in this change, so it should be able pack the types properly, but there may be other cases. Maybe something to look into down the line. The only slight caveat is it has been worthwhile shrinking these because of the upcoming change to model multi-bucket features will duplicate them per time series model. I suspect we may have smaller wins elsewhere.

[edsavage]

A bit late to the party but LGTM