[Run3 PromptReco] Many memory allocations CaloSubdetectorGeometry::cellGeomPtr()

[makortel]

Continuing from #46040 (comment). The CaloSubdetectorGeometry::cellGeomPtr() leads to 1.44 million allocations per event on average in Run 3 Prompt Reco. From GitHub search I found my earlier analysis in #42672 (comment), where the main question was if the function

cmssw/Geometry/CaloGeometry/src/CaloSubdetectorGeometry.cc

Lines 260 to 265 in fe85c4c

	std::shared_ptr<const CaloCellGeometry> CaloSubdetectorGeometry::cellGeomPtr(uint32_t index) const {
	// Default version
	auto ptr = getGeometryRawPtr(index);
	static const auto do_not_delete = [](const void*) {};
	return ptr == nullptr ? nullptr : std::shared_ptr<const CaloCellGeometry>(ptr, do_not_delete);
	}

could be changed to return const CaloCellGeometry* directly instead of a shared_ptr?

[makortel]

assign Geometry/CaloGeometry

[cmsbuild]

New categories assigned: geometry

@bsunanda,@civanch,@Dr15Jones,@kpedro88,@makortel,@mdhildreth you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @makortel.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

type performance-improvements

[makortel]

I see the interface was changed in #21808 from returning a raw pointer to returning a shared_ptr.

[VinInn]

I fail to understand the difference between std::shared_ptr(ptr, do_not_delete); and a raw pointer

[makortel]

std::shared_ptr needs to hold the reference count somewhere (it doesn't know what the deleter does). I crafted a simple test program

#include <memory>
#include <iostream>

int const* get1() {
  static const int val = 1;
  return &val;
}

std::shared_ptr<int const> get2() {
  static const int val = 1;
  static const auto do_not_delete = [](const void*) {};
  return std::shared_ptr<int const>(&val, do_not_delete);
}

std::shared_ptr<int const> get3() {
  static const int val = 1;
  return std::make_shared<int>(val);
}

int main() {
  int sum = 0;
  constexpr int N = 1000;
  for (int i=0; i<N; ++i) {
    auto ptr = get2();
    sum += *ptr;
  }
  std::cout << sum << std::endl;

  return 0;
}

compiled with g++ -std=c++20 -O3 test.cc -o test and ran through the MaxMemoryPreload AllocMonitor showing

• returning raw pointer, get1(): # allocations calls: 0
• returning shared_ptr with do_not_delete, get2(): # allocations calls: 1000
• returning shared_ptr from make_shared, get3(): # allocations calls: 1000

[makortel]

I think this API is problematic in any case. If cellGeomPtr() is intended to be called (many times) during the event processing, creating truly owning shared_ptrs would be even more expensive.

Then if cellGeomPtr() is not intended to be called event processing, there is substantial amount of re-engineering to be done (see callers from here).

[makortel]

#46507 has a proposal for reducing the number of memory allocations

[makortel]

I think this API is problematic in any case. If cellGeomPtr() is intended to be called (many times) during the event processing, creating truly owning shared_ptrs would be even more expensive

Just to note the memory cost of the "truly owning shared_ptrs`" is visible in #46511 (O(100 MB) / event in Phase 2 RelVal).