Implement committed output - #124
Merged
Merged
Conversation
Define code structure and developer contract Work needs to be done. Needs testing. Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Plus changed some docstring wording Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Added test for committed output in presence of stragglers. Requires 2 threads. Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Signed-off-by: AdrianoPi <24545691+AdrianoPi@users.noreply.github.com>
Signed-off-by: Alessandro Pellegrini <a.pellegrini@ing.uniroma2.it> # Conflicts: # src/gvt/fossil.c # src/lp/process.c # src/mm/msg_allocator.h
alessandropellegrini
force-pushed
the
feat/committed_output
branch
from
June 25, 2026 13:36
8e8cff2 to
843fbb0
Compare
This commit avoids accessing output buffers if not necessary. Also, output buffers are initialized upon message creation. Signed-off-by: Alessandro Pellegrini <a.pellegrini@ing.uniroma2.it>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #124 +/- ##
===========================================
- Coverage 85.51% 85.41% -0.10%
===========================================
Files 46 47 +1
Lines 1553 1742 +189
Branches 39 46 +7
===========================================
+ Hits 1328 1488 +160
- Misses 209 235 +26
- Partials 16 19 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Three bugs are fixed in the committed output implementation: - msg_allocator_alloc() did not initialize msg->outputs to NULL. Since mm_alloc() wraps malloc(), freshly allocated messages could contain a garbage pointer in that field. All functions guarding on if(!outputs) would then skip the NULL check and dereference the garbage value, leading to a crash or memory corruption. - The outputs field in struct lp_msg was placed between pl_size and pl[], inside the MPI-transmitted region. When a message was sent to a remote node the local pointer value was serialized, and the receiving side would overwrite the field with that garbage value via MPI_Mrecv. The field is moved into the message preamble (before dest) so it is never transmitted. - fossil_lp_collect() called execute_outputs() on every PES entry, including PES_ENTRY_SENT_LOCAL ones. Sent messages do not carry outputs; only received messages do. The call is moved after the sent-local guard and the already-extracted m pointer is reused to avoid a redundant pes_entry_msg() call. Two additional correctness issues are also addressed in output.c: - The silent_processing guard in ScheduleOutput() was checked after the serial fast-path. This meant that calling ScheduleOutput() during LP_FINI in serial mode would incorrectly fire the output callback, contrary to what the API documentation states. The guard is now checked first. - The serial mode detection used the deprecated global_config.serial boolean instead of global_config.synchronization == SERIAL, leading to lose all output: ScheduleOutput() would buffer entries on current_msg->outputs that execute_outputs() never flushes in serial mode. The committed_output test is extended with: - A serial mode test case, verifying that the output callback fires immediately and in causal order. - A total output count assertion in the Time Warp test that detects any phantom outputs from events that were rolled back but whose outputs were not correctly suppressed by committed_output_on_rollback(). - Per-invocation output_size verification in the output callback. - Bounds checking on the output buffer before writing to it. Signed-off-by: Alessandro Pellegrini <a.pellegrini@ing.uniroma2.it>
…/committed_output
In the fixing process, I started adding a store that is now unnecessary given the other fixes. Remove it. Signed-off-by: Alessandro Pellegrini <a.pellegrini@ing.uniroma2.it>
Member
|
@AdrianoPi I have rebased the PR againts the current code base. I have made some changes (mostly documented in commit messages), it could be great if you could quickly recheck! |
alessandropellegrini
marked this pull request as ready for review
June 28, 2026 08:57
alessandropellegrini
requested review from
Piccions and
alessandropellegrini
as code owners
June 28, 2026 08:57
Collaborator
Author
|
Thanks! msg->outputs = NULL;in The rest looks good! |
In the fixing process, I started adding a store that is now unnecessary given the other fixes. Remove it. Signed-off-by: Alessandro Pellegrini <a.pellegrini@ing.uniroma2.it>
Member
|
Good catch! I have removed that. |
Piccions
reviewed
Jul 1, 2026
Piccions
left a comment
Piccions
left a comment
Contributor
There was a problem hiding this comment.
LGTM, just a minor change suggested
Reversing the order in which the output messages are materialized upon fossil collection ensures a per-LP order. There is no global order yet, as fossil_lp_collect() is executed lazily and possibly concurrently by multiple workers. Signed-off-by: Alessandro Pellegrini <a.pellegrini@ing.uniroma2.it>
Piccions
approved these changes
Jul 2, 2026
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
3 participants
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.
This pull request adds a committed output management system to the simulation kernel.
This lets simulation models schedule output operations while handling an event. Scheduled operations are then carried out when the originating event is committed, guaranteeing exactly-once execution.
Implementation changes
core/output.cand headercore/output.himplementing committed output management.ROOT-Sim.hto expose new functionScheduleOutput(), which lets the user submit data for handling upon event commitment.struct simulation_configurationto take a user-defined handlerPerformOutput_tthat receives the data submitted viaScheduleOutput()with exactly-once semantics once the generating event is committed.struct lp_msgto store committed output data.current_msgis now used by the committed output system and always needs to be set.Testing
test/core/output.c) that verifies correct output sequence in the presence of straggler messages.