Skip to content

Conversation

@makortel
Copy link
Contributor

@makortel makortel commented Dec 12, 2025

PR description:

ROOT team informed us that the auto_ptr read rule recipe applied to a test in #48817 contains a memory leak. The memory leak occurs when the objects are reused when reading, which is not how PoolSource works. This PR fixes the memory leak. (I opened it as a draft because presently it seems to have a leak).

Related to #43422, #43923
Resolves cms-sw/framework-team#1718

PR validation:

The test runs. I modified the test to repeat the the 10-event file for 1000 times, and ran it through the MaxMemoryPreload. The code in master seems to have a ~42 B/event leak, but so seems to have the present version of this PR. Adding back the direct schema evolution from auto_ptr->unique_ptr does not leak. See #49621 (comment).

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 12, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@makortel
Copy link
Contributor Author

@cmsbuild, please test

Just to demonstrate it technically works.

@makortel
Copy link
Contributor Author

@pcanal Do you see any obvious mistakes wrt. what you suggested in #49593 or the example in ROOT repository?

@pcanal
Copy link
Contributor

pcanal commented Dec 12, 2025

Its look good.

@makortel
Copy link
Contributor Author

Ok. This version just seems to leak at similar rate as the version in master.

@pcanal
Copy link
Contributor

pcanal commented Dec 12, 2025

Are you able to tell where the leaked memory is allocated?

@makortel
Copy link
Contributor Author

Not presently :(

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4f1106/49933/summary.html
COMMIT: c304144
CMSSW: CMSSW_16_0_X_2025-12-12-1100/el8_amd64_gcc13
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/49621/49933/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 4 lines from the logs
  • Reco comparison results: 8 differences found in the comparisons
  • Reco comparison had 4 failed jobs
  • DQMHistoTests: Total files compared: 53
  • DQMHistoTests: Total histograms compared: 4273241
  • DQMHistoTests: Total failures: 50
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4273171
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 52 files compared)
  • Checked 227 log files, 198 edm output root files, 53 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor Author

Ok, mystery understood (thanks @Dr15Jones). The memory leak of the present code in master occurs if the deprecated_auto_ptr is reused (as the comment I copied from ROOT also states). But PoolSource in cmsRun does not reuse the objects, but creates a new Wrapper for every read, and that likely explains why I don't see any difference with this PR in a cmsRun-based test.

I made a simple test program on bare ROOT

#include "TFile.h"
#include "TTree.h"

int main() {
  auto* file = TFile::Open(".../IOPool/Input/data/SchemaEvolutionTestOLD15_1_0_pre5_splitLevel99.root");
  auto* events = (TTree*)file->Get("Events");
  for (int j=0; j<1000; ++j) {
    for (int i=0; i<10; ++i) {
      events->GetEntry(i);
    }
  }
  return 0;
}

and with that the MaxMemoryPreload shows a clear increase in the memory (and outstanding memory allocations) when increasing the iteration limit of the outer loop in the present code. With this PR the memory increase is gone.

@makortel makortel marked this pull request as ready for review December 12, 2025 19:42
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel for master.

It involves the following packages:

  • DataFormats/TestObjects (core)

@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@missirol, @mmusich, @rovere, @wddgit this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

Comparison differences are related to #47071

@makortel
Copy link
Contributor Author

+core

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @mandrenguyen, @ftenchini (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit fced6f4 into cms-sw:master Dec 15, 2025
10 checks passed
@makortel makortel deleted the autoPtrMemFix branch December 15, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix memory leak

4 participants