Clean up source and build system.#151
Open
luciansmith wants to merge 49 commits into
Open
Conversation
NSBML was removed from the antimony source yonks ago; this cleans it out of auxiliary files.
Required for the latest google test, but probably a good idea in general.
Also compile that with debug info, so any error message will be at least somewhat informative. Also allow manual running of valgrind on the debug branch when we need more diagnostic information.
Also upload all of them as an artifact, and split into groups of 20 so we see them before waiting the entire time.
We were inconsistently deleting a passed-around vector<string>* object from antimony.ypp. Now we pass it as a const reference instead, and delete it inside antimony.ypp directly, instead of inside the functions it passed to. Also actually delete the removed rateOf function definition when it was found in the SBML model.
Fixed dependency leaks in libsbml (and a now-unused fix in libsbmlnetwork). Also only upload memory test results, not all of them, and give them unique names.
Add CI that runs things through valgrind.
GetVariable() fell back to a full linear scan of m_variables whenever m_varmap missed, re-deriving by string comparison something the map lookup had already ruled out for any variable this module owns directly. m_varmap is populated by StoreVariable() on every insertion, and the only place a variable's name can change after being stored (Module::SetNewTopName) already clears and rebuilds the whole map immediately afterward -- so a miss on a plain (non-qualified) name is never a stale-cache problem. The recursive submodule search in the same loop is not redundant: it's how qualified names (e.g. paths into a comp:Submodel) get resolved the first time, since composite keys are only cached into m_varmap lazily after a successful recursive lookup. That behavior is preserved unchanged here -- only the linear GetName() comparison against every variable is removed. On yeast-GEM.xml (a large flat FBC model with zero comp:Submodel usage), this fallback was measured at ~13.5s across 68,263 calls out of a ~50s total load -- the single largest identified contributor to slow loading of large models, since every not-yet-seen variable name during loading, and every existing-variable relookup during Module::Finalize, paid for a full rescan of everything added so far. Benchmarked with a standalone timing harness (not included in this commit) built specifically to isolate this from other load-time costs; see the investigation notes for the full breakdown.
The previous fix removed the redundant name comparison from GetVariable()'s map-miss fallback, but the loop still scanned every variable in m_variables to find the (usually zero) entries of type varModule, since that's the only way to know which ones are submodule instances worth recursing into. Still O(n) per miss, just with a much cheaper per-iteration cost. This adds Module::m_submoduleVars, a side list containing only the variables that are (or were) type varModule, maintained incrementally: - Module::StoreVariable() appends to it when a variable is inserted already typed as varModule (e.g. copied from m_defaultVariables). - Variable::SetModule() -- the only place in the codebase a variable ever transitions to varModule -- calls the new Module::NoteSubmoduleVariable() the first time that happens. GetVariable() (both the const and non-const overloads) now scans m_submoduleVars instead of m_variables, making the fallback O(number of submodules this module has ever had) rather than O(total variables) -- O(1) in practice for the many models with none. Entries are never removed from the list, so both overloads keep a live GetType() == varModule guard on each entry in case one is later deleted/retyped; this can't produce an incorrect result, only a harmless extra iteration over an already-small list. On yeast-GEM.xml, combined with the previous fix, this took GetVariable's fallback from ~13.5s/68,263 calls down to ~2ms, and total load time from ~54s to ~31s -- the fix also incidentally recovered several seconds each from Module::Finalize's Phase 3 (SetComponentCompartments) and Phase 4 (substance unit creation), both of which turned out to be paying the old O(n) GetVariable cost indirectly rather than having their own bottleneck.
Getvariable linear scan fix
Finalize() previously called checkConsistency() on both m_sbml and a throwaway reparsed copy (testdoc), then again inside fixFBCStrictIfNeeded() -- redundant and costly on large models. Now checkConsistency() is called once, on testdoc only. If it reports errors, fixFBCStrictIfNeeded(testdoc) is invoked; if the model has FBC and needs strict relaxed, it sets strict=false directly on m_sbml's model (fixing a bug where the flag was set on the throwaway document instead of the persisted one), and removes the specific FBC strict-conditional error IDs (2020608, 2020707-2020716) from testdoc's error log rather than clearing it wholesale, so genuine parse errors are still caught. Error-processing loop extracted into Module::ProcessSBMLErrorLog for reuse and clarity. Adds test_simple_flux_comp_strict, which checks the hierarchical (comp=true) SBML output directly -- the code path that exposed the 'strict set on wrong document' bug, since getSBMLString's comp=false path rebuilds from scratch and would mask it.
Deduplicate checkConsistency() calls and fix FBC strict-flag handling
…omp) Both functions previously rescanned all of m_variables/m_uniquevars on every call: a full vector copy, an erase-based pointer filter for the comp=true case, then a linear AreEquivalent() scan. CreateSBMLModel calls them in the universal 'n = GetNumVariablesOfType(...); for i in 0..n: GetNthVariableOfType(..., i, ...)' pattern, so a single section on a model with n variables cost O(n) calls x O(n) work = O(n^2). On the 20k-variable yeast-GEM model this was by far the largest single cost in the whole load (~11.4s across 22,836 calls). Added Module::GetVariablesOfTypeCached(rtype, comp), which memoizes the exact same scan (same order, same filtering) keyed by (rtype, comp), and made GetNumVariablesOfType/GetNthConstVariableOfType thin wrappers around it. The cache is invalidated in the two places that can change what it holds: Finalize() clears it up front (m_uniquevars is rebuilt fresh every Finalize(), same as it always was), and StoreVariable() clears it on every variable insertion -- StoreVariable is the common chokepoint every m_variables/m_uniquevars mutation already funnels through, so this covers new variables created mid- CreateSBMLModel (e.g. AddOrFindUnitDef) without needing to track mutations at every call site individually. Result: GetNthVariableOfType dropped from ~11.4s to ~2ms across the same call count in benchmarking on yeast-GEM.xml, with no behavior change intended -- this is a pure memoization of the prior scan.
Cache GetNumVariablesOfType/GetNthVariableOfType results
Annotated::TransferAnnotationTo() re-derives XHTML notes from markdown via SBase::setNotesFromMarkdown() every time it's called, but on a comp/hierarchical model each annotated object gets this call at least twice (once for the comp=true SBML build, once for comp=false), always producing the same HTML from the same markdown. On yeast-GEM.xml this was one of the largest single costs in the whole load (~3s across 5163 calls). Added Annotated::m_notesHTML, a cache of the HTML/XHTML corresponding to m_notes: - ReadAnnotationFrom stashes the original HTML read from SBML (via getNotesString()) alongside the existing markdown conversion, when it's establishing the sole notes fragment for this object. - TransferAnnotationTo checks the cache first and feeds it straight through setNotes() if present, skipping getNotesString()'s join and the markdown conversion entirely. On a cache miss it falls back to the existing logic unchanged, then stores the freshly-generated HTML for next time. - AppendNotes (notes set by parsing an Antimony script, independent of any SBML notes element) clears the cache, since it no longer corresponds to the current text. - Synchronize copies the cache alongside m_notes when merging two variables, so a synchronized variable doesn't lose it. Result: TransferAnnotationTo's Notes handling dropped from ~3s to ~470ms in benchmarking on yeast-GEM.xml, with no behavior change intended for the generated SBML.
Shorten long comment.
FixNames() checks the model against ~130 reserved Antimony keywords/functions/constants/unit names, previously via FixConstants(name, model) and FixFunctions(name, model) for each one -- 268 calls total, each calling Model::getElementBySId(name), which does a full recursive walk of the whole model tree to confirm a reserved word isn't in use (the common case, since collisions are rare). On yeast-GEM.xml (~40k SBML elements) this cost ~3s on its own. Replaced with a single pass: call getAllElements() once, build a set<string> of the ~130 reserved words once, then walk every element checking its ID against that set (skipping immediately if the element has no ID at all). That pass itself hit a second, unrelated O(n^2): libSBML's List has no iterator, and List::get(n) walks from the head every time, so the straightforward 'for (el = 0; el < elements->getSize(); el++) elements->get(el)' loop is O(n^2) over a linked list. Replaced with 'while (elements->getSize() > 0) elements->remove(0)', which drains the list head-first in O(1) per removal. Applied the same fix to the (rare-path, only-runs-on-an-actual-collision) rename loops inside the renamed FixConstant/FixFunction, which have the identical shape. FixConstants/FixFunctions renamed to FixConstant/FixFunction and now take the already-found element directly instead of re-deriving it via another getElementBySId() call -- they're only ever called from FixNames, so this is a safe, contained signature change. Result: LoadSBML:FixNames dropped from ~3s to ~25ms in benchmarking on yeast-GEM.xml, with no behavior change intended.
Notes html cache
Rewrites FixNames to walk the model once via getAllElements() and check each element against the reserved-word set, instead of calling getElementBySId() once per reserved word (~130 calls, each an O(n) tree walk). FixConstants/FixFunctions become FixConstant/FixFunction, taking the already-found element directly instead of re-finding it. Also excludes SBML element types whose ids live outside the core SId namespace (UnitDefinition, LocalParameter, comp:Port, and -- guarded by the corresponding LIBSBML_HAS_PACKAGE_* macros -- arrays/multi/spatial package elements), since a generic getAllElements() walk sees them but the old getElementBySId()-based code never did, and renaming them would be incorrect (e.g. Antimony's own global-unit syntax).
…ntimony into fixnames-quadratic-fix
Speedup FixNames
Don't load it several times; don't re-translate something we already have the translation for.
Speed up QTAntimony.
Also, exchangetest2 never existed at all; we can safely remove it entirely.
Note from Claude: Windows 11 has a known taskbar regression: WM_SETICON's ICON_SMALL updates the window's titlebar/Alt-Tab/jump-list icon fine, but silently fails to update the taskbar button itself if the icon handle passed is too small (e.g. the ~16-24px pixmap Qt normally sends for ICON_SMALL). The workaround is to send a large icon (up to 256x256) for *both* ICON_BIG and ICON_SMALL. This matches exactly what was reported: correct icon in the window/jump list, generic icon on the taskbar button. Qt's setWindowIcon() above still matters for the general/titlebar/Alt-Tab icon; this native call specifically targets the taskbar. See: https://learn.microsoft.com/en-us/answers/questions/712281/wm-seticon-does-not-update-taskbar-icon-in-windows
The model "species S1 in C; C = S1 + 3" would trigger a libsbml validation error, since you can't calculate C without knowing S1's concentration, and you can't know S1's concentration without knowing C.
Not sure how I didn't notice this earlier!
There's a potential race condition where finding the tests could be not finished before the tests started to run. Ran into this once on mac/linux. This should fix it.
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.
No description provided.