Conversation
Building and destroying a FireDomain with a propagation layer cost about
183 kB every time, linearly and without limit. Two things were missing an
owner, and a third made ownership impossible.
The propagative layer was never freed: the delete in ~FireDomain was
commented out and the pointer nulled, which also removed any chance of a
later cleanup finding it.
The models were never freed either. propModelsTable and fluxModelsTable
are static, shared by every domain in the process, and nothing cleared
them. Each domain now records the entries it registered and releases
exactly those, before the data broker goes, since the models hold a
pointer to it.
The reason that could not simply be added: the FireDomain constructor
wiped both tables. Building a second domain therefore dropped the first
one's registrations, and its PropagativeLayer was left holding an index
that was empty until the new domain registered — at which point it
resolved to the *second* domain's model. Command.cpp builds exactly that
second domain for coupled runs. The wipe is gone; both tables are static
storage and already start out null.
Two smaller things on the same path:
- getFreePropModelIndex counted down through an unsigned index with no
lower bound, so a full table wrapped to SIZE_MAX and read far out of
bounds. getFreeFluxModelIndex bound-checked but then returned an
occupied index, silently overwriting a live model. Both now report
and return an out-of-range value that their callers refuse.
- DataBroker::addConstantLayer allocated an array, handed it to a layer
that copies it, and dropped it. Both delete[] lines were
sitting there commented out, one per branch.
Measured over 500 domains, each with a Rothermel layer: RSS growth falls
from 91,504 kB to 180 kB, and occupied model slots from perpetually
climbing to zero after each domain. Under ASan the unit suite goes from
4.7 MB leaked to none, so the leak check is now blocking for it; runff
still leaks ~200 kB on paths the suite does not reach and stays
informational.
tests/unit/test_domain_ownership.cpp covers the release, the
cross-domain clobbering and the loop. The model registry tests no longer
delete models by hand, which is a double free now that the domain owns
them; each takes its own sandbox instead, so destruction is exercised
the way it actually happens.
Closes #159
|
Looks good: fixes the leak and the cross-domain clobbering, tests cover both. Minor, non-blocking: same missing "table full" check in |
|
Noted for |
|
hi @HugoFara please give me to Oct 20th or so to test in coupled fire/atmospher context. these are changes in memory management that needs to be done, but that may have other consequences |
|
No problem @filippi . I do the testing with Meso-NH 6.1 as I'm also working with the team there. What checks do you need to run? I may be able to offload some of the stress-tests. |
|
hi @HugoFara well.. I didn't even tried with MesoNH 6.1.. if the pedrogao test passes it should be allright. |
|
Thanks; I'm working on wildfire models out of interest, it is not associated with my current university position. Aurélien also gave me the list of things to finish on Blaze. Build: Meso-NH loads ForeFire from |
This branch has not been deployed
3 participants
Closes #159.
A
FireDomainwith a propagation layer leaked ~183 kB each time, with no upper limit (500 domains: +91.5 MB RSS before, +180 kB after).The fix is not just the missing
delete. The constructor also wiped the static model tables, so building a second domain (Command.cpp:205does this in coupled runs) left the first domain with a null slot, or with the second domain's model, which silently gives wrong spread rates.getFreePropModelIndex/getFreeFluxModelIndex, and a leaked array inDataBroker::addConstantLayer.Tested: new
tests/unit/test_domain_ownership.cpp(each half of the fix has a negative control). Unit tests 5/5.runffmatches. ASan leaks drop from 4.7 MB to 0, sosanitizers.ymlnow runs withdetect_leaks=1and blocks on leaks (unblocks #162).Code and description generated with Claude Opus 5, reviewed before submitting.