Argos collector integration for map_v3 - #679
Conversation
* Enhancements and fixes for CI testing (#606) * Run Buffer_test in github * Fix cmake code for macos and have Buffer_test emit errors * Temporarily disable ccache in github runners * Pin the runner to use macos-13. Things started failing with AppleClang 17. * Re-enable ccache for github CI * Add continue-on-error to macos build yml * Go back to using macos-15 but disable ccache temporarily * Refactor valgrind setup cmake code * Redirect SpartaTester errors to a log file for CI * Disable Buffer_test on MacOS * Database-backed checkpointer (#605) * Refactor Sparta checkpointers * Database-backed checkpointer * Database-backed checkpointer * Database-backed checkpointer * Database-backed checkpointer * Database-backed checkpointer * Database-backed checkpointer * Database-backed checkpointer * Database-backed checkpointer * Last set of changes before redesign * Database-backed checkpointer * Database-backed checkpointer * Update all doxygen and add more testing * Fix clang build/regress failures * Fix clang build/regress failures * Attempt to fix -LE issue with ctest (macos) * Align the apple/valgrind/ctest code in the test dir and example dir * Redirect std::cerr to a logfile for the Buffer_test * Address PR feedback * Address PR feedback * Remove old debug code * Add DatabaseCheckpointer::findLatestCheckpointAtOrBefore() * Remove O(n) code from ensureWindowLoaded_ * Re-enable Buffer_test * Move DatabaseCheckpoint test dir * Change variable name in unit test * Bump SimDB * Bump SimDB * PR feedback * Bump SimDB
* Add AI-produced tests for sparta::Queue's iterators * Fix AI's mistake * Clarify new tests * Whoops, simplified test does not actually demonstrate bug I care about. Add back a targetted regression test that shows the bug. * Fix the bug * I don't like that double decrement from begin() goes to a valid element. Let's be strict instead. * More robust test-- might as well push another commit to see if CI is happier this time
I'll tag map_v2.1.5 when merged.
This is needed for CoSim's use of the DatabaseCheckpointer. That app has a non-default ctor taking the ArchData root TreeNode and the scheduler. While the scheduler is the same for all instances (one checkpointer instance for each core/hart), the ArchData root is different.
As windows are processed, we only allow them to exist in exactly one spot: the cache, the pipeline, or the database. There are asserts in the code that ensure no duplicate windows are found, which is incorrectly firing in some use cases. Windows that are "deleted" in the pipeline (e.g. deleting future windows during `loadCheckpoint`) just get a flag "ignore=true" to save time. Finding "dups" in the pipeline, where e.g. 1 is valid and 3 are ignored, should not count as 4 dups.
Each Pegasus hart gets its own checkpointer which already captures all memory/registers under e.g. top.core0.hart0 But the PegasusSystem (top.system) has its own MemoryObject's (ArchData) which also have to be added to the checkpointers or else we can't checkpoint those ArchData's. Adding "top.system" to the checkpointers brings up the CoSim ISA test pass rate to 96%.
This PR redesigns the DatabaseCheckpointer since there are sporadic failures seen in the CoSim test harness. There are more/less failures depending on how you parameterize the harness (how many N steps to take before flushing to N-1, how big of a cache to use, and how big of a snapshot threshold to use). The previous design fundamentally worked like this: - LRU main cache, pipeline, DB - Checkpoints can only exist in one place out of the above three - Reloading a checkpoint from the pipeline will remove it from the pipeline and add it back to the cache - Reloading a checkpoint from the DB will delete the record from the DB and add it back to the cache - Reloading a checkpoint from anywhere deletes all future checkpoints since it doesn't allow >1 branch - Checkpoint IDs must be auto-incrementing and contiguous / no gaps The constant reloading / deleting future checkpoints is not a stable design. All sporadic failures come down to exceptions from not being able to find a given checkpoint ID. Making this a stable design would require more thread contention / mutex locks and performance would not be good. The reason there is a 100% pass rate with the default harness parameters is that the pipeline is not very deep, and the cache is big enough to avoid so many things being reloaded into the LRU cache and deleted from the pipeline / DB. Change the parameters to deep pipelines or a smaller cache, and all the bugs appear. This new design is very different, but has many advantages: - Full reuse of the FastCheckpointer to implement the cache. Multiple branches allowed. - Cache has no size limit and is only cleared with an explicit API call. - All CoSim flush operations will be done from the cache guaranteed. Much faster, no contention. - When you are ready to move the simulation forward, pick one of the branches / checkpoints from the FastCheckpointer, and let those be committed down the pipeline. All checkpoints not committed to the pipeline will just be removed from the FastCheckpointer. - Think of the cache as a staging area for git workflows. Create branches, commit changes, delete unused branches, move on. - Use non-contiguous checkpoint IDs (which contain those from branches NOT taken) and arch IDs (these are now the contiguous IDs only from checkpoints / branches taken and sent to the pipeline). - Limitation: Once you commit checkpoints (remove from cache), you cannot flush back to those checkpoints. But you are in control of how big the cache gets before clearing it with the API call. We could relax this limitation in the future, but for CoSim we will never have a need to flush to an older event that was already committed. So this is not a limitation for Pegasus. I am wrapping up the follow-up PR for Pegasus which integrates this checkpointer. Previously, the pass rate could be anywhere from 30% to 85% for some harness configurations, and the CoSim pass rate is now 100% for all harness configurations I've tried. One last note: I can follow up with another PR that renames CherryPickFastCheckpointer back to DatabaseCheckpointer, but I did it this way or the PR diff would be too hard to read since everything changed. But there is also the PersistentFastCheckpointer, which sounds the same as DatabaseCheckpointer, so I thought having CherryPickFastCheckpointer makes sense since it's the only one that has this behavior of pulling checkpoints / branches out of the FastCheckpointer.
Co-authored-by: Knute Lingaard <56927209+klingaard@users.noreply.github.com> Co-authored-by: Knute Lingaard (MIPS) <155678575+knute-mips@users.noreply.github.com>
Need pipeline snoopers for CoSim event recreation performance. Note that this was already a feature in SimDB, then the pipelines got redesigned. I'm adding it back for CoSim which currently uses no snoopers. When looking for something not in the event cache, it currently needs to flush the whole pipeline to the database and query / recreate the event from disk (slow). Snooping the pipeline stages in memory is much faster, unless the requested event is so old that it is in the database already. For most use cases, we should be able to get the event from the boost serialization or zlib stages without flushing or going to disk. Next I'll release map_v2.1.12 which should be the last release we need for Pegasus' basic cosim functionality.
This PR removes some of the SimDB implementation details from user-written pipeline stages. This is the last thing I wanted to clean up before finalizing the full pipeline documentation - I'm going to add a README file for it (different than the SimDB main README).
Made sure we're compiling with systemc 2.3. Also cleaned up documentation a bit and added more descriptions.
Cherry picked: ``` commit b78c3ed (HEAD -> master, origin/master, origin/HEAD) Author: colby-nyce <iheartridleyman@gmail.com> Date: Sun Feb 1 12:55:47 2026 -0600 TreeNode extensions bug fixes (#642) commit b8b1700 Author: colby-nyce <iheartridleyman@gmail.com> Date: Fri Jan 30 12:56:00 2026 -0600 Add --simdb-file cmdline option (#641) commit 62c3888 Author: colby-nyce <iheartridleyman@gmail.com> Date: Thu Jan 29 11:05:53 2026 -0600 CommandLineSimulator: multiple app instances (#640) ```
Seems that gcc let this issue slide but clang does not. Even without calling `addExtension<T>()`, clang can fail to build our internal simulators since `ParameterSet` is not defined in `TreeNodeExtensionManager.hpp` where `make_unique<ParameterSet>()` is called. Note also the circular include which prevents `ParameterSet.hpp` from being included by `TreeNodeExtensionManager.hpp`: ``` TreeNode.hpp --> TreeNodeExtensionManager.hpp ----> ParameterSet.hpp ------> TreeNode.hpp ``` Co-authored-by: Colby Nyce <cnyce@mips.com>
There was a problem hiding this comment.
I've been banging my head against some collection bugs and noticed an issue here, so since you're already under the hood...
This specialization doesn't actually work because the std::is_same clause is checking if the pointer type (T) is the same as the function argument type (H). The same bug is present in several places, which I'll note in other comments with "pointer bug here".
Once this is fixed, we then need to deal with a new problem, namely what do we want to do if we pass a null pointer to the collector?
There was a problem hiding this comment.
Thanks for calling this out @bdutro-mips. How does the bug surface? Is it a wrong answer bug, or is it not collected at all, is it a crash, or does the wrong populateFromEntityUtility_ get invoked? I would need a simple example using PairDefinition and SPARTA_ADDPAIR that draws the bug out. Even pseudo code will help. I just need to see an example of a collected data type that would hit this bug. If it's a hard crash, then a repro sim command would probably be sufficient.
Last question - is this an existing bug that you found using the v2 collector, or did you just find this while using my branch?
There was a problem hiding this comment.
How does the bug surface? Is it a wrong answer bug, or is it not collected at all, is it a crash, or does the wrong
populateFromEntityUtility_get invoked?
One of the other populateFromEntityUtility_ overloads is invoked (specifically one where the object type does not match the function/method type), which results in no collection if you pass a pointer to the collector without dereferencing it.
Something as simple as this should cover it:
class APairDef;
struct A
{
using SpartaPairDefinitionType = APairDef;
uint32_t getID() const { return 42; }
};
class APairDef
{
APairDef() : PairDefinition<A>() { SPARTA_INVOKE_PAIRS(A); }
SPARTA_REGISTER_PAIRS(SPARTA_ADDPAIR("id", &A::getID))
};
// later during simulation...
auto a = std::make_unique<A>(); // shared_ptr and SpartaSharedPointer should also trigger this bug
a_collector.collectWithDuration(a, 1);
I think the above version might only surface with Collectable objects, since the IterableCollector works a little differently. However, you can also trigger it with IterableCollectors if you SPARTA_FLATTEN a method that returns a pointer:
class BPairDef;
struct B
{
using SpartaPairDefinitionType = BPairDef;
std::unique_ptr a{new A};
const auto& getA() const { return a; }
};
class BPairDef
{
BPairDef() : PairDefinition<B>() { SPARTA_INVOKE_PAIRS(B); }
SPARTA_REGISTER_PAIRS(SPARTA_FLATTEN(&B::getA))
};
Last question - is this an existing bug that you found using the v2 collector, or did you just find this while using my branch?
It's an existing bug in the v2 collector
There was a problem hiding this comment.
@bdutro-mips I'm not seeing any weird behavior related to these bugs in the v3 collector (this PR branch). I did however see some head scratchers during development where a no-op populateFromEntityUtility_ was being called, thus silently being unable to collect certain fields. I also saw a bug where calling SPARTA_FLATTEN on a method returning a pointer was unable to collect (I forget the bad code path so I don't know if it's the same bug you are seeing, but IIRC the field wasn't even flattened at all, just skipped over entirely, resulting in the front-end python code not being able to parse the bytes due to layout mismatches). Those were legitimate bugs with the v2 collector and were fixed in this PR. Not 100% sure if we are talking about the same bugs though.
Check out the new populateFromEntityUtility_ methods that I added in this file, and check out the diff for Collectable.hpp to see if these changes seem like they would fix these bugs. If you have time, the best would be to run collection using this branch and see if the same issues persist. Otherwise I'm not sure what else I would be able to do at this point.
There was a problem hiding this comment.
Ok, it sounds like you already ran into the issue then. Do you think it would be possible to remove the no-op populateFromEntityUtility_ handlers and make them error at compile time or run time? That way we can avoid weirdness like this in the future.
There was a problem hiding this comment.
I'm not sure we should do that since all this SpartaKeyPairs code is also used for PEvents. Removing any of the no-op handlers causes the pair collection tests to stop compiling (ambiguous overloads). What I can do is differentiate between PEvents/Argos at runtime in the no-op handlers and throw if they are ever hit when using this code for Argos. Last thing I want to do is risk destabilizing map_v3 PEvents out of the gate.
There was a problem hiding this comment.
Is there a use-case for no-op PEvent handlers? That sounds like another bug to me. However, I realize that's outside the original scope of this PR, so we could defer it for later.
| Clone sparta: | ||
| ``` | ||
| git clone --recursive git@github.com:sparcians/map --branch map_v2.2 | ||
| git clone --recursive git@github.com:sparcians/map --branch map_v3 |
There was a problem hiding this comment.
Might want to have folks stick with v2 for now until simulators like Olympia are transitioned. I'd like to consider map_v2.2 the most stable branch right now
| | Development prior to SimDB integration | SimDB integration/report generation support | TreeNode Extensions API Update | | ||
| | map_v2.0 | map_v2.1 | map_v2.2 | map_v3.0 | | ||
| | -------------------------------------- | ------------------------------------------- | -------- | -------- | | ||
| | Development prior to SimDB integration | SimDB integration/report generation support | TreeNode Extensions API Update | Redesigned Argos backed by SimDB | |
There was a problem hiding this comment.
It's more than a redesign -- it's a brand new Argos!!!
79c4813 to
e9363fb
Compare
Co-authored-by: Knute Lingaard <klingaard@gmail.com> Co-authored-by: Victor A. Ying <ying.victor@gmail.com> Co-authored-by: Knute Lingaard <56927209+klingaard@users.noreply.github.com> Co-authored-by: Knute Lingaard (MIPS) <155678575+knute-mips@users.noreply.github.com> Co-authored-by: Colby Nyce <cnyce@mips.com> 73ddbc7
6 participants
No description provided.