root icon indicating copy to clipboard operation
root copied to clipboard

[PyROOT] Memory leak in _TDirectoryFile_Get

Open silverweed opened this issue 1 year ago • 2 comments

Check duplicate issues.

  • [X] Checked for duplicates

Description

The pythonization of TDirectoryFile has a _TDirectoryFile_Get function (used to implement Get()) that may return a TObject whose memory is not owned by either c++ or python, causing a memory leak.

I attempted to fix the issue with a simple ROOT.SetOwnership, but that causes some tests to fail, specifically pyunittests-pyroot-pyz-tdirectoryfile-attrsyntax-get.

Reproducer

  1. in python, open a TFile containing many different objects (e.g. histograms)
  2. Get all those objects using file.Get(name) in a for loop (relinquishing their reference at every iteration)
  3. see the memory increase forever.

You can test the bug with the following script:

import resource
import ROOT

def print_memory_usage(message):
    print(f"{message:50} {resource.getrusage(resource.RUSAGE_SELF).ru_maxrss}")

print_memory_usage("start")
histogram_names = open("histogram_names.txt").read().splitlines()
print_memory_usage("read histogram names")

fn = "NTUP_PHYSVAL.40023485._000001.pool.root.1"
with ROOT.TFile.Open(fn) as f:
    print_memory_usage("open ROOT file")
    for i, histogram_name in enumerate(sorted(histogram_names)):
        h = f.Get(histogram_name)
        if i % 1000 == 0:
            print_memory_usage(f"read {i+1} histograms")
    print_memory_usage("read all histograms")
print_memory_usage("outside context maneger (closing ROOT file)")

The files used were provided by the user who reported the issue, see the forum post

ROOT version

master

Installation method

built from source

Operating system

Linux

Additional context

This bug was reported on the forum

silverweed avatar Aug 06 '24 15:08 silverweed

This also affects ROOT 6.30 right? So it's not related to the recent PyROOT developments like the cppyy upgrade.

guitargeek avatar Aug 06 '24 16:08 guitargeek

@guitargeek yes, it affects also 6.30.

silverweed avatar Aug 07 '24 06:08 silverweed