(0039900)
Stephen Kelly (developer)
2015-11-21 11:18

Prior to commit 25e04ddf the PopDefinitions() method would destroy a cmDefinitions element along with the memory resources it controlled. That commit changed to use a data structure (cmLinkedTree) which is not designed to have non-leaf nodes removed from it.

That means that when a function scope is exited, that used to free up memory prior to that commit, but after that commit the memory is not freed up.

It would be possible to remove the nodes from the cmLinkedTrees in the cmState implementation on Pop(), but perhaps the better fix is to leave the structure intact but clear the definitions themselves from all snapshots from the beginning to the end of the function, such as (untested):


  diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
  index 4fb4579..33f87a9 100644
  --- a/Source/cmMakefile.cxx
  +++ b/Source/cmMakefile.cxx
  @@ -5149,10 +5149,13 @@ cmMakefile::FunctionPushPop::FunctionPushPop(cmMakefile* mf,
     : Makefile(mf), ReportError(true)
   {
     this->Makefile->PushFunctionScope(fileName, pm);
  + this->Snp = this->Makefile->GetStateSnapshot();
   }

   cmMakefile::FunctionPushPop::~FunctionPushPop()
   {
  + this->Makefile->GetState()->ClearState(this->Makefile->GetStateSnapshot(),
  + this->Snp);
     this->Makefile->PopFunctionScope(this->ReportError);
   }

  diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h
  index f1dd374..627bb34 100644
  --- a/Source/cmMakefile.h
  +++ b/Source/cmMakefile.h
  @@ -672,6 +672,7 @@ public:
       void Quiet() { this->ReportError = false; }
     private:
       cmMakefile* Makefile;
  + cmState::Snapshot Snp;
       bool ReportError;
     };

  diff --git a/Source/cmState.cxx b/Source/cmState.cxx
  index c491c7d..989abb6 100644
  --- a/Source/cmState.cxx
  +++ b/Source/cmState.cxx
  @@ -275,6 +275,20 @@ bool cmState::GetCacheEntryPropertyAsBool(std::string const& key,
                ->GetCacheIterator(key.c_str()).GetPropertyAsBool(propertyName);
   }

  +void cmState::ClearState(Snapshot beginSnp, Snapshot endSnp)
  +{
  + PositionType begin = beginSnp.Position;
  + PositionType end = endSnp.Position;
  +
  + PositionType it = begin;
  +
  + for ( ; it != end; ++it)
  + {
  + *begin.Position->Vars = cmDefinitions();
  + }
  + *begin.Position->Vars = cmDefinitions();
  +}
  +
   void cmState::AddCacheEntry(const std::string& key, const char* value,
                                       const char* helpString,
                                       cmState::CacheEntryType type)


This way, it should be easy to add a mode in the future which does not clear the definitions (or any other versioned state that cmState gains in the future), but makes it available for tooling.

(0039903)
Brad King (manager)
2015-11-23 13:22

Re 0015854:0039900: That helps somewhat, but the problem is deeper. Consider this case:

$ cat example.cmake
function(nothing)
endfunction()

foreach(n RANGE 1 10000000)
nothing()
endforeach()

Prior to cmState snapshotting the loop could run an arbitrary number of iterations in constant memory usage. Now we grow memory usage for every function call because cmState::CreateFunctionCallSnapshot allocates resources that are not freed when the function returns.

(0039904)
Brad King (manager)
2015-11-23 13:30

Meanwhile I've added a test case covering add_subdirectory inside a function:

Tests: Add case for add_subdirectory inside a function
https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=010c5959 [^]

This represents a funny scoping case that needs to be preserved.

(0039905)
Brad King (manager)
2015-11-23 13:35

Re 0015854:0039903: I think function calls are too granular for the state snapshotting, at least without an explicit debugging option enabled. The resource accumulation cost is not reasonable or bounded.

If when a function call returns we see that no nested cmLinkedTree entries are left (SnapshotData, VarTree, ExecutionListFiles, others?) then those associated with the call should be removed. This way no resources accumulate for finished function calls, but scopes needed for cases like add_subdirectory inside a function call stick around.

(0039921)
Brad King (manager)
2015-11-30 11:27

Here are some changes to avoid accumulating storage for short-lived scopes:

Merge topic 'reduce-cmState-accumulation-for-master'
https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=3c6a3668 [^]

More work will be needed later to address snapshot lifetimes more carefully. We should avoid keeping entire snapshots just because a cmListFileBacktrace was temporarily constructed for a diagnostic message. Furthermore, long-lived backtraces should not have to hold on to their entire scope snapshots but instead just enough to have the backtrace info.

(0039930)
Brad King (manager)
2015-12-01 13:22

Here are some examples that use very little memory after the fix but lots of memory before the fix:

$ cat example-1.cmake
function(concat v l r)
  set(${v} "${l};${r}" PARENT_SCOPE)
endfunction()

set(o "")
foreach(a RANGE 1 16)
  foreach(b RANGE 1 16)
    foreach(c RANGE 1 16)
      foreach(d RANGE 1 16)
        concat(o "${o}" "a")
      endforeach()
    endforeach()
  endforeach()
endforeach()

$ cat example-2.cmake
function(nothing)
endfunction()

foreach(a RANGE 1 16)
  foreach(b RANGE 1 16)
    foreach(c RANGE 1 16)
      foreach(d RANGE 1 16)
        foreach(e RANGE 1 16)
          foreach(f RANGE 1 16)
            nothing()
          endforeach()
        endforeach()
      endforeach()
    endforeach()
  endforeach()
endforeach()

(0039931)
Brad King (manager)
2015-12-01 13:23

The fix linked in 0015854:0039921 has been merged to 'release' for inclusion in 3.4.1.

(0041270)
Kitware Robot (administrator)
2016-06-10 14:21

This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.

Issue History
Date Modified	Username	Field	Change
2015-11-19 14:16	Brad King	New Issue
2015-11-19 14:16	Brad King	Status	new => assigned
2015-11-19 14:16	Brad King	Assigned To	=> Stephen Kelly
2015-11-19 14:16	Brad King	Target Version	=> CMake 3.5
2015-11-21 11:18	Stephen Kelly	Note Added: 0039900
2015-11-23 13:22	Brad King	Note Added: 0039903
2015-11-23 13:30	Brad King	Note Added: 0039904
2015-11-23 13:35	Brad King	Note Added: 0039905
2015-11-24 15:40	Brad King	Assigned To	Stephen Kelly => Brad King
2015-11-30 11:27	Brad King	Note Added: 0039921
2015-11-30 11:27	Brad King	Target Version	CMake 3.5 => CMake 3.4.1
2015-12-01 13:22	Brad King	Note Added: 0039930
2015-12-01 13:23	Brad King	Note Added: 0039931
2015-12-01 13:23	Brad King	Status	assigned => resolved
2015-12-01 13:23	Brad King	Resolution	open => fixed
2015-12-01 13:23	Brad King	Fixed in Version	=> CMake 3.4.1
2016-06-10 14:21	Kitware Robot	Note Added: 0041270
2016-06-10 14:21	Kitware Robot	Status	resolved => closed

Notes
(0039900) Stephen Kelly (developer) 2015-11-21 11:18	Prior to commit 25e04ddf the PopDefinitions() method would destroy a cmDefinitions element along with the memory resources it controlled. That commit changed to use a data structure (cmLinkedTree) which is not designed to have non-leaf nodes removed from it. That means that when a function scope is exited, that used to free up memory prior to that commit, but after that commit the memory is not freed up. It would be possible to remove the nodes from the cmLinkedTrees in the cmState implementation on Pop(), but perhaps the better fix is to leave the structure intact but clear the definitions themselves from all snapshots from the beginning to the end of the function, such as (untested): diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 4fb4579..33f87a9 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -5149,10 +5149,13 @@ cmMakefile::FunctionPushPop::FunctionPushPop(cmMakefile* mf, : Makefile(mf), ReportError(true) { this->Makefile->PushFunctionScope(fileName, pm); + this->Snp = this->Makefile->GetStateSnapshot(); } cmMakefile::FunctionPushPop::~FunctionPushPop() { + this->Makefile->GetState()->ClearState(this->Makefile->GetStateSnapshot(), + this->Snp); this->Makefile->PopFunctionScope(this->ReportError); } diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index f1dd374..627bb34 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -672,6 +672,7 @@ public: void Quiet() { this->ReportError = false; } private: cmMakefile* Makefile; + cmState::Snapshot Snp; bool ReportError; }; diff --git a/Source/cmState.cxx b/Source/cmState.cxx index c491c7d..989abb6 100644 --- a/Source/cmState.cxx +++ b/Source/cmState.cxx @@ -275,6 +275,20 @@ bool cmState::GetCacheEntryPropertyAsBool(std::string const& key, ->GetCacheIterator(key.c_str()).GetPropertyAsBool(propertyName); } +void cmState::ClearState(Snapshot beginSnp, Snapshot endSnp) +{ + PositionType begin = beginSnp.Position; + PositionType end = endSnp.Position; + + PositionType it = begin; + + for ( ; it != end; ++it) + { + begin.Position->Vars = cmDefinitions(); + } + begin.Position->Vars = cmDefinitions(); +} + void cmState::AddCacheEntry(const std::string& key, const char* value, const char* helpString, cmState::CacheEntryType type) This way, it should be easy to add a mode in the future which does not clear the definitions (or any other versioned state that cmState gains in the future), but makes it available for tooling.

(0039903) Brad King (manager) 2015-11-23 13:22	Re 0015854:0039900: That helps somewhat, but the problem is deeper. Consider this case: $ cat example.cmake function(nothing) endfunction() foreach(n RANGE 1 10000000) nothing() endforeach() Prior to cmState snapshotting the loop could run an arbitrary number of iterations in constant memory usage. Now we grow memory usage for every function call because cmState::CreateFunctionCallSnapshot allocates resources that are not freed when the function returns.

(0039904) Brad King (manager) 2015-11-23 13:30	Meanwhile I've added a test case covering add_subdirectory inside a function: Tests: Add case for add_subdirectory inside a function https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=010c5959 [^] This represents a funny scoping case that needs to be preserved.

(0039905) Brad King (manager) 2015-11-23 13:35	Re 0015854:0039903: I think function calls are too granular for the state snapshotting, at least without an explicit debugging option enabled. The resource accumulation cost is not reasonable or bounded. If when a function call returns we see that no nested cmLinkedTree entries are left (SnapshotData, VarTree, ExecutionListFiles, others?) then those associated with the call should be removed. This way no resources accumulate for finished function calls, but scopes needed for cases like add_subdirectory inside a function call stick around.

(0039921) Brad King (manager) 2015-11-30 11:27	Here are some changes to avoid accumulating storage for short-lived scopes: Merge topic 'reduce-cmState-accumulation-for-master' https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=3c6a3668 [^] More work will be needed later to address snapshot lifetimes more carefully. We should avoid keeping entire snapshots just because a cmListFileBacktrace was temporarily constructed for a diagnostic message. Furthermore, long-lived backtraces should not have to hold on to their entire scope snapshots but instead just enough to have the backtrace info.

(0039930) Brad King (manager) 2015-12-01 13:22	Here are some examples that use very little memory after the fix but lots of memory before the fix: $ cat example-1.cmake function(concat v l r) set(${v} "${l};${r}" PARENT_SCOPE) endfunction() set(o "") foreach(a RANGE 1 16) foreach(b RANGE 1 16) foreach(c RANGE 1 16) foreach(d RANGE 1 16) concat(o "${o}" "a") endforeach() endforeach() endforeach() endforeach() $ cat example-2.cmake function(nothing) endfunction() foreach(a RANGE 1 16) foreach(b RANGE 1 16) foreach(c RANGE 1 16) foreach(d RANGE 1 16) foreach(e RANGE 1 16) foreach(f RANGE 1 16) nothing() endforeach() endforeach() endforeach() endforeach() endforeach() endforeach()

(0039931) Brad King (manager) 2015-12-01 13:23	The fix linked in 0015854:0039921 has been merged to 'release' for inclusion in 3.4.1.

(0041270) Kitware Robot (administrator) 2016-06-10 14:21	This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.

Relationships