Keep track of allocation and release memory (faster than by calling mallinfo and more accurate than calling footprint)

[palainp]

This PR adds a counter to memory usage (simply malloc calls add the size value, and free calls substract the size value). The tricky part is that dmalloc can use dispose_chunk to do something similar to free and we have to substract memory used there too.

This doesn't change the API and just adds an exported function malloc_memory_usage (I'm not sure that's the best name though).

[hannesm]

Thanks for this PR. I was wondering whether the subtraction could be done at a single place, dispose_chunk, since it looks like with this PR it happens just before every call site thereof?

[palainp]

Thanks for this PR. I was wondering whether the subtraction could be done at a single place, dispose_chunk, since it looks like with this PR it happens just before every call site thereof?

You're totally right about that, this comes from my tests to find out why the value of internal_memory_usage can goes down the real usage and unfortunately I changed two things at once:

• removing static and adding volatile qualifier for internal_memory_usage
• the extraction of internal_memory_usage -= ... from dispose_chunk But it appears that I now have the substraction before every dispose_chunk calls. I'll test out with a single place modification at the begining of dispose_chunk.

[palainp]

After testing (intensive network usage + 32M unikernel which is barely enough, to try to have the worst possible situation) I'm not able to reproduce any difference between the value returned by mallinfo and the internal_memory_usage value. It's better to declare it as static and with code factorization it is easier to read.

[palainp]

I think I need some advice here, looking back to the patch, with regard to the following paragraph https://github.com/mirage/ocaml-solo5/blob/80ff24ef8f678e2b2f7c6c8dc45e59caf739a0cb/nolibc/dlmalloc.i#L2990 it may be better to move the internal_memory_usage inside the global state: https://github.com/mirage/ocaml-solo5/blob/80ff24ef8f678e2b2f7c6c8dc45e59caf739a0cb/nolibc/dlmalloc.i#L2587

[hannesm]

To me this looks fine. Only one thing: could we at the top of dlmalloc.i have a brief comment that this version of dlmalloc.i was extended with dlmalloc_memory_usage / malloc_state with the field internal_memory_usage to safe ourselves some headache when we want to upgrade dlamalloc.i at some point.

Of course another review would be nice before a merge and release.

[palainp]

I hope this notice comment will be clear and explanatory enough.

As a note, could it be possible to co-author @winux138 for e9c5343 and 80ff24e who worked on that too ? I'm not sure I have done that correctly.

[hannesm]

This looks fine to me. I also looked into the difference of assembly output between main branch and these changes, and there is not much difference (some subs etc., but not any more function calls etc.).

As a note, could it be possible to co-author @winux138 for e9c5343 and 80ff24e who worked on that too ? I'm not sure I have done that correctly.

You did it at least partially correct, what works is the line in a commit message Co-authored-by: NAME <email-address>

Now, you only put "@winux138" there which relies on GitHub to translate the user ID to a link -- if you put the eMail address there instead, it should work (and be independent of GitHub as a plaform).

Would you also mind to squash all commits into a single one?

[hannesm]

Thanks a lot!