Give the propagation models and layers an owner - #182
Open
HugoFara wants to merge 1 commit into
Open
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #159. A
FireDomainwith a propagation layer leaked ~183 kB per construct/destroy cycle, linearly and without limit. Over 500 domains, each attaching a Rothermel layer:Why the missing
deletecould not just be addedThe
FireDomainconstructor wiped both static model tables:propModelsTableis static and shared process-wide, so building a second domain dropped the first one's registrations. Domain A'sPropagativeLayerkeeps index 49 throughout: once B is constructed that slot is null andFireDomain.cpp:1349dereferences it unchecked, and once B registers, it resolves to B's model — so A propagates with another domain's parameters, silently, with wrong spread rates rather than a crash.Command.cpp:205builds exactly that second domain for coupled runs.The wipe is gone. Both tables are static storage and already start null, and each domain now records the entries it registered and releases exactly those — before the data broker, since the models hold a pointer to it. Two smaller ownership fixes ride along: the free-index scans in
getFreePropModelIndexandgetFreeFluxModelIndex, and a dropped array inDataBroker::addConstantLayer.Verification
Unit suite 5/5, including new
tests/unit/test_domain_ownership.cpp, with a negative control per half of the fix: restoring the constructor wipe fails the clobbering case, removing the release call fails the slot case.runffKML and NetCDF both match within tolerance. Under ASan the unit suite goes from 4.7 MB leaked to none, sosanitizers.ymlnow runs it withdetect_leaks=1as a blocking check — what #162 was waiting for.This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.
EDIT: rewrote for human readability.