G4World stores raw G4Volume* wrappers in g4volumesMap but never deletes them. Each g4world.reset() (every geometry reload) leaks one G4Volume wrapper per volume.
Affected files
gemc/g4system/g4world.h:211 (std::unordered_map<std::string, G4Volume*> g4volumesMap;, no destructor)
gemc/g4system/g4objectsFactories/g4objectsFactory.h:176 ((*g4s)[volume_name] = new G4Volume();)
gemc/gdetector/gdetectorConstruction.cc:74 (g4world.reset();)
Details
Wrappers are allocated with new and inserted into the map:
// g4objectsFactory.h:173-178
G4Volume* getOrCreateG4Volume(const std::string& volume_name, std::unordered_map<std::string, G4Volume*>* g4s) {
if (auto it = g4s->find(volume_name); it != g4s->end()) { return it->second; }
else {
(*g4s)[volume_name] = new G4Volume();
return (*g4s)[volume_name];
}
}
G4World has no destructor and holds them as raw pointers:
// g4world.h:211
std::unordered_map<std::string, G4Volume*> g4volumesMap;
When GDetectorConstruction::Construct() rebuilds geometry it does:
// gdetectorConstruction.cc:73-74
gworld.reset();
g4world.reset();
g4world.reset() destroys the G4World (its shared_ptr use count drops to 0), the map is destroyed, but the raw G4Volume* it held are never deleted — they leak. One wrapper leaks per volume, on every reload.
Ownership boundary: the actual Geant4 objects (G4VSolid, G4LogicalVolume, G4VPhysicalVolume) are owned by Geant4's internal stores, not by the G4Volume wrapper, so only the lightweight GEMC G4Volume wrapper is leaked — the fix must delete the wrapper only, never the contained G4 objects.
Impact
A per-volume memory leak that recurs on every geometry reload. For a large SoLID-scale geometry reloaded many times in an interactive session, this is a meaningful and unbounded growth.
Proposed fix: have the map own the wrappers via std::unique_ptr
- /** Map "gsystem/volumeName" → G4Volume wrapper pointer. */
- std::unordered_map<std::string, G4Volume*> g4volumesMap;
+ /** Map "gsystem/volumeName" → owned G4Volume wrapper. */
+ std::unordered_map<std::string, std::unique_ptr<G4Volume>> g4volumesMap;
This requires updating the few sites that traffic in raw pointers, since several helpers pass std::unordered_map<std::string, G4Volume*>*:
getOrCreateG4Volume (g4objectsFactory.h:176) and the getSolidFromMap / getLogicalFromMap / getPhysicalFromMap helpers (g4objectsFactory.h:152-161) take std::unordered_map<std::string, G4Volume*>*. They would need to take the owning-map type and return it->second.get().
get_g4volumes_map() (g4world.h:127) returns a copy of unordered_map<…, G4Volume*>; with unique_ptr it must instead return a map of raw observer pointers (build and return {name, ptr.get()}), which is what callers like GTree already expect.
Lower-churn alternative (keeps the raw-pointer map type and all helper signatures): add a destructor to G4World that deletes the wrappers.
G4World(const GWorld* gworld, const std::shared_ptr<GOptions>& gopts);
+
+ /// Deletes the owned G4Volume wrappers. Geant4 owns the contained
+ /// solid/logical/physical objects, so only the wrappers are freed here.
+ ~G4World() {
+ for (auto& [name, vol] : g4volumesMap) { delete vol; }
+ }
Given the number of helper signatures that pass the raw-pointer map around, the destructor approach is the smaller, lower-risk change; the unique_ptr approach is cleaner if the helper signatures are updated together.
Split out from #102 (one issue per bug). Found via an AI-assisted code review with manual verification against commit 5f8ce875.
G4Worldstores rawG4Volume*wrappers ing4volumesMapbut never deletes them. Eachg4world.reset()(every geometry reload) leaks oneG4Volumewrapper per volume.Affected files
gemc/g4system/g4world.h:211(std::unordered_map<std::string, G4Volume*> g4volumesMap;, no destructor)gemc/g4system/g4objectsFactories/g4objectsFactory.h:176((*g4s)[volume_name] = new G4Volume();)gemc/gdetector/gdetectorConstruction.cc:74(g4world.reset();)Details
Wrappers are allocated with
newand inserted into the map:G4Worldhas no destructor and holds them as raw pointers:// g4world.h:211 std::unordered_map<std::string, G4Volume*> g4volumesMap;When
GDetectorConstruction::Construct()rebuilds geometry it does:// gdetectorConstruction.cc:73-74 gworld.reset(); g4world.reset();g4world.reset()destroys theG4World(itsshared_ptruse count drops to 0), the map is destroyed, but the rawG4Volume*it held are neverdeleted — they leak. One wrapper leaks per volume, on every reload.Ownership boundary: the actual Geant4 objects (
G4VSolid,G4LogicalVolume,G4VPhysicalVolume) are owned by Geant4's internal stores, not by theG4Volumewrapper, so only the lightweight GEMCG4Volumewrapper is leaked — the fix must delete the wrapper only, never the contained G4 objects.Impact
A per-volume memory leak that recurs on every geometry reload. For a large SoLID-scale geometry reloaded many times in an interactive session, this is a meaningful and unbounded growth.
Proposed fix: have the map own the wrappers via
std::unique_ptrThis requires updating the few sites that traffic in raw pointers, since several helpers pass
std::unordered_map<std::string, G4Volume*>*:getOrCreateG4Volume(g4objectsFactory.h:176) and thegetSolidFromMap/getLogicalFromMap/getPhysicalFromMaphelpers (g4objectsFactory.h:152-161) takestd::unordered_map<std::string, G4Volume*>*. They would need to take the owning-map type and returnit->second.get().get_g4volumes_map()(g4world.h:127) returns a copy ofunordered_map<…, G4Volume*>; with unique_ptr it must instead return a map of raw observer pointers (build and return{name, ptr.get()}), which is what callers likeGTreealready expect.Lower-churn alternative (keeps the raw-pointer map type and all helper signatures): add a destructor to
G4Worldthat deletes the wrappers.Given the number of helper signatures that pass the raw-pointer map around, the destructor approach is the smaller, lower-risk change; the
unique_ptrapproach is cleaner if the helper signatures are updated together.Split out from #102 (one issue per bug). Found via an AI-assisted code review with manual verification against commit
5f8ce875.