Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved timers #272

Merged
merged 8 commits into from
Apr 12, 2024
Merged

Improved timers #272

merged 8 commits into from
Apr 12, 2024

Conversation

mdorier
Copy link
Contributor

@mdorier mdorier commented Apr 10, 2024

This PR improves the timers API in a few ways:

  • It adds margo_timer_create_with_pool to specify the pool in which the timer's callback should be submitted as a ULT. If ABT_POOL_NULL is passed, the callback will be executed in the context of the progress loop. I added a warning in the function's documentation that this should be avoided and has limitations (such as not being allowed to call any margo functions in the callback since they could deadlock).
  • It changes the semantics of margo_timer_cancel(). Previously, this function would only remove the timer from the list of future timers, but would not cancel a ULT submitted by the timer. Now it does, and it also waits for completion of any ULT submitted by the timer. This guarantees that after margo_timer_cancel() returns, the timer's callback will not be called, hence any data associated with the callback can safely be freed.
  • It changes the semantics of margo_timer_destroy(). Previously, this function would cancel the timer and free its memory. Now it does not cancel the timer. If the timer hasn't started and hasn't submitted its ULT, it is going to free the timer's memory. If it has been started or if it has a pending ULT, it is going to mark the timer for deletion and the last ULT using the timer will destroy it.
  • It ensures, upon finalizing the margo instance, that no timer remains pending.
  • It cleans up the internal implementation of timers: before, we had duplicate code paths, with functions like __margo_timer_init being used from inside margo and margo_timer_create being used from outside. The distinction is now gone, and several of the __margo_timer_* function have been removed as they were redundant with the public version.
  • There are unit tests meant to test possible scenarios (such as cancelling a timer at different point in time, destroying it before it's fired, etc.).

@mdorier mdorier requested a review from carns April 10, 2024 12:13
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 81.16883% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 57.17%. Comparing base (765fef2) to head (d981396).

Files Patch % Lines
src/margo-timer.c 73.11% 10 Missing and 15 partials ⚠️
src/margo-core.c 75.00% 0 Missing and 3 partials ⚠️
src/margo-init.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #272       +/-   ##
===========================================
+ Coverage   24.22%   57.17%   +32.94%     
===========================================
  Files          70       69        -1     
  Lines        9539     9548        +9     
  Branches     1257     1242       -15     
===========================================
+ Hits         2311     5459     +3148     
+ Misses       7116     3331     -3785     
- Partials      112      758      +646     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@carns carns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great and I like the changes. Just one minor comment on correcting a function description.

* @brief Cancel a started timer.
* @brief Cancel a started timer. If the timer's callback
* has already been submitted as a ULT, this ULT will
* eventually be executed. Hence, calling margo_timer_cancel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function comment is incorrect. There is no wait_pending() function. By my reading of the code it will block until timer ULTs are complete (if they have been launched already). I like that because it is much simpler anyway.

In place of the note about wait_pending() it should probably warn callers that this particular could block (in an Argobots-safe way, that is).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I forgot to update this comment.

@mdorier
Copy link
Contributor Author

mdorier commented Apr 12, 2024

I added a margo_timer_cancel_many and corresponding unit test, and corrected the comment. Merging.

@mdorier mdorier merged commit 64ad2f8 into main Apr 12, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants