Skip to content

Commit

Permalink
[df] Avoid memory leak in JIT when the execution is not triggered
Browse files Browse the repository at this point in the history
If a node of the computation graph needs JITting, it will allocate objects on
the heap which are only freed if the JITting happens. In case the computation
graph is never triggered, previously the code to be JITted was not run. Fix it
by making sure that at destruction time an RDataFrame calls the JIT compilation
through its RLoopManager. Also, move to a better read/write locking with the
more modern ROOT::gCoreMutex.

Fixes root-project#15399
  • Loading branch information
vepadulano committed May 2, 2024
1 parent 5e30c29 commit 22c7722
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 9 deletions.
8 changes: 8 additions & 0 deletions tree/dataframe/inc/ROOT/RDataFrame.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ public:
RDataFrame(ULong64_t numEntries);
RDataFrame(std::unique_ptr<ROOT::RDF::RDataSource>, const ColumnNames_t &defaultColumns = {});
RDataFrame(ROOT::RDF::Experimental::RDatasetSpec spec);

// Rule of five

RDataFrame(const RDataFrame &) = default;
RDataFrame &operator=(const RDataFrame &) = default;
RDataFrame(RDataFrame &&) = default;
RDataFrame &operator=(RDataFrame &&) = default;
~RDataFrame();
};

namespace RDF {
Expand Down
11 changes: 11 additions & 0 deletions tree/dataframe/src/RDataFrame.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -1666,6 +1666,17 @@ RDataFrame::RDataFrame(ROOT::RDF::Experimental::RDatasetSpec spec)
{
}

RDataFrame::~RDataFrame()
{
// If any node of the computation graph associated with this RDataFrame
// declared code to jit, we need to make sure the compilation actually
// happens. For example, a jitted Define could have been booked but
// if the computation graph is not actually run then the code of the
// Define node is not jitted. This in turn would cause memory leaks.
// See https://github.com/root-project/root/issues/15399
fLoopManager->Jit();
}

namespace RDF {
namespace Experimental {

Expand Down
22 changes: 13 additions & 9 deletions tree/dataframe/src/RLoopManager.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include "TEntryList.h"
#include "TFile.h"
#include "TFriendElement.h"
#include "TROOT.h" // IsImplicitMTEnabled
#include "TROOT.h" // IsImplicitMTEnabled, gCoreMutex, R__*_LOCKGUARD
#include "TTreeReader.h"
#include "TTree.h" // For MaxTreeSizeRAII. Revert when #6640 will be solved.

Expand Down Expand Up @@ -803,15 +803,19 @@ void RLoopManager::CleanUpTask(TTreeReader *r, unsigned int slot)
/// This method also clears the contents of GetCodeToJit().
void RLoopManager::Jit()
{
// TODO this should be a read lock unless we find GetCodeToJit non-empty
R__LOCKGUARD(gROOTMutex);

const std::string code = std::move(GetCodeToJit());
if (code.empty()) {
R__LOG_INFO(RDFLogChannel()) << "Nothing to jit and execute.";
return;
{
R__READ_LOCKGUARD(ROOT::gCoreMutex);
if (GetCodeToJit().empty()) {
R__LOG_INFO(RDFLogChannel()) << "Nothing to jit and execute.";
return;
}
}

const std::string code = []() {
R__WRITE_LOCKGUARD(ROOT::gCoreMutex);
return std::move(GetCodeToJit());
}();

TStopwatch s;
s.Start();
RDFInternal::InterpreterCalc(code, "RLoopManager::Run");
Expand Down Expand Up @@ -978,7 +982,7 @@ void RLoopManager::SetTree(std::shared_ptr<TTree> tree)

void RLoopManager::ToJitExec(const std::string &code) const
{
R__LOCKGUARD(gROOTMutex);
R__WRITE_LOCKGUARD(ROOT::gCoreMutex);
GetCodeToJit().append(code);
}

Expand Down

0 comments on commit 22c7722

Please sign in to comment.