Skip to content

[Low] Make G4World own its G4Volume wrappers to stop a leak per volume on geometry reload #135

@zhaozhiwen

Description

@zhaozhiwen

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    Status
    Planned

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions