Introducing class and methods to log memory usage in scope

[BjarneHerland]

Issue
Resolves #2353

Approach
Introduced some methods to report different aspects of memory usage. Wrapped methods by RAII to log mem-usage in process on entering and exiting scope. Example of usage in smoother_update

[BjarneHerland]

@dotfloat your input on the approach would be appreciated in particular about how to test this in an effivient way.

[codecov-commenter]

Codecov Report

Merging #2640 (4aa1e23) into main (f3e8aa0) will decrease coverage by 0.09%.
The diff coverage is 21.91%.

@@            Coverage Diff             @@
##             main    #2640      +/-   ##
==========================================
- Coverage   64.90%   64.80%   -0.10%     
==========================================
  Files         649      651       +2     
  Lines       53841    53897      +56     
  Branches     4540     4609      +69     
==========================================
- Hits        34944    34927      -17     
- Misses      17443    17507      +64     
- Partials     1454     1463       +9     

Impacted Files	Coverage Δ
libres/lib/analysis/update.cpp	0.00% <0.00%> (ø)
libres/lib/include/ert/res_util/memory.hpp	0.00% <0.00%> (ø)
libres/lib/include/ert/logging.hpp	29.41% <29.41%> (ø)
libres/lib/res_util/memory.cpp	42.30% <42.30%> (ø)
ert_gui/ertwidgets/validationsupport.py	79.45% <0.00%> (-19.18%)	⬇️
res/job_queue/forward_model_status.py	88.75% <0.00%> (-5.00%)	⬇️
ert_shared/status/tracker/legacy.py	85.18% <0.00%> (-2.65%)	⬇️
ert_shared/ensemble_evaluator/config.py	95.40% <0.00%> (-2.30%)	⬇️
ert_gui/ertwidgets/__init__.py	75.00% <0.00%> (-2.28%)	⬇️
ert_shared/models/base_run_model.py	84.58% <0.00%> (-0.40%)	⬇️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e8aa0...4aa1e23. Read the comment docs.

[joakim-hove]

The purpose of this extern "C" is to avoid name mangling - i.e. to get "simple" symbol names which can be accessed from the cwrap machinery. Unless you intend to instantiate this class from Python I would suggest removing the extern "C"

[BjarneHerland]

Good point

[joakim-hove]

Suggestion: I would create a small struct:

struct meminfo {
    long current; 
    long max;

    meminfo();
}

with the implementation of the constructor in the .cpp file. The benefits from this:

1. You codify the fact that you are interested in a snapshot of the memory situation - not individual independent values.
2. You avoid exporting the SystemRamFree(),.... functions (export a struct instead through).
3. You can test the OS call functions by creating memino instance - without offering public access into the logger class.

For the member variable in the logger I would avoid the use of cur or similar abbreviations for current, when that information is used in the destructor it represents the initial memory information.

My previous experience with memory logging like this is that it is difficult to get conclusive results, unless you are doing something really extreme (e.g. spikes of 98% memory usage) the OS will be in there and do it's thing. I think memory reporting by the kernel is a topic which has been worked on - so hopefully you will have more success.

[BjarneHerland]

Mmmm... guess we have slightly different views on some aspects here... 😄

The SystemRawFree()... etc are meant to be exposed as utilities. Assuming they produce correct results, they may be useful in developing code which adapts to memory-requirements, not only for logging.

The class utilizes the RAII-promise of C++ to easily "checkpoint" mem-usage and gain insight into how a code-block (scope) uses memory (note logging of max-mem). I.e. we log current and max mem-usage on entering a scope and the same on exiting the scope. Thus, if max-mem changed significally we know the the scope allocated (and hopefully released) a lot of memory, providing insight without the need for analyzing code in detail.

I'll have a look at the prefix of member-vars - names may be clearer, yes...

Finally, I completely agree on the complexity of determining memory usage in general. This is an initlal approach which I believe we can get some useful info from it. It's a shame this will only work in Linux, but I guess that is fair enough for real life.

[BjarneHerland]

Verified manually by pytest -sv tests/libres_tests/res/enkf/test_es_update.py

[joakim-hove]

I am honored to be requested for review; I have some suggestions/opinions. I do not expect you to follow them, and in particular I do not intend to discuss these discussion further - unless explicitly encouraged to do so. If you think the ideas have some merit - that is good; otherwise no harm done.

[joakim-hove]

Mmmm... guess we have slightly different views on some aspects here...

I am a bit uncertain if you understood what I meant - so just to be certain we are discussing the same suggestion I'll spell it out in some detail:

//memory.hpp
struct meminfo {
   long process_memory;
   long max_memory;
   meminfo();
}

class ScopedMemoryLogger {
public:
     ScopedMemoryLogger(logger_type& logger, const std::string& msg)
       : m_logger(logger)
       , m_init_mem(meminfo{})
     {}

    ~ScopedMemoryLogger();

private:
      logger_type& logger;
      meminfo m_init_mem;
};    


// memory.cpp
meminfo::meminfo() 
    :  max_memory( ProcessMaxMemory() )
    ,  process_memory( ProcessMemory() )
{}

The advantage of this is:

1. You do not need to publish the ProcessMaxMemory() family of functions. As we have already discussed this type of memory profiling is inherently difficult - my hunch is that these functions will not be used outside of the ScopedMemoryLogger - no need to publish the symbols prematurely (OK: you obviously do not need the struct meminfo in order to hide these symbols).
2. It is in my opinion nice and clean to collect the mem information in a struct - instead of naked long values. YMMV though.

[BjarneHerland]

@joakim-hove I appreciate your input as much as as your pragmatism. Upcoming patch will definitely contain some of your suggestions!

[pinkwah]

Test ideas:

1. Allocate 1MB (auto data = new char[1<<20];), instantiate the scoped memory logger, delete[] data;, then check that the memory has been reduced.
2. Instantiate scoped memory logger, allocate 1MB, check that memory has increased, delete[] data;
3. Allocate process_max_memory() - process_memory() + 1MB, do the above.

Note: Allocation is very complex so these tests may give false negatives. Linux only sees the pages that the malloc has allocated, but malloc has it's own heap and it doesn't necessarily need to release the pages when you free(). So it could be that malloc finds a megabyte of contiguous space in its own heap and gives you that, and so the kernel will give you the same memory stats.

[BjarneHerland]

test ert please

[BjarneHerland]

test libres please

[BjarneHerland]

test ctest please

[BjarneHerland]

Thanks for ideas @pinkwah! I agree that this will be hard to test deterministically...

Have we established some way to mock C/C++ functions using Catch2? We could mock process_max_memory() and process_memory() , then verify that scoped_memory_logger calls the ert::Llogger correctly..?

Or can we venture into (ab)using LD_PRELOAD to control what's read from the /proc-filesystem?

[BjarneHerland]

Thanks for ideas @pinkwah! I agree that this will be hard to test deterministically...

Have we established some way to mock C/C++ functions using Catch2? We could mock process_max_memory() and process_memory() , then verify that scoped_memory_logger calls the ert::Llogger correctly..?

Or can we venture into (ab)using LD_PRELOAD to control what's read from the /proc-filesystem?

The test in this latest patch is deterministic and I think it is sufficient.

[BjarneHerland]

test ert please

[BjarneHerland]

test ert please

[BjarneHerland]

test this please

[pinkwah]

Minor revisiony stuff. Looks pretty good otherwise!

[pinkwah]

💯 good job!