Refactor xevent #1622
Merged
Refactor xevent #1622
Conversation
This file contains 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
This necessitates some changes: * Extending the callbacks with an "xpack" parameter that was available to the special-cased functions for sending data out from a handler. * Making synchronisation between execution of the callback and deleting the event optional, to not introduce (unnecessary) blocking where it wasn't there before. * Changing the closure argument to a blob that is copied into the event, rather than an arbitrary pointer. The problem is a need to avoid races with the garbage collector which makes it impossible to store the pointer to an entity without introducing additional stages in the deletion process. This in turn forces an additional indirection in the "old" callbacks. * The internal functions heavily relied on the availability of a "gv" pointer, this could easily be emulated via the closure argument, but it seems to make more sense to pass it as an argument. This allows moving a lot of handler code to more reasonable places, which in turn made "ddsi_discovery" explode too messy to survive without splitting it. Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
They really are no more than conditional queueing of a particular message, and that is all intimately tied to proxy endpoints and the matching thereof. Constructing the message in the proxy endpoint code and simply queueing it is arguably cleaner, and it removes all dependencies on (proxy) entity implementation details from the event queue. Signed-off-by: Erik Boasson <eb@ilities.com>
This moves the "schedule time rounding" into the sleeping a bit longer in ddsrt_cond_waitfor, which should be a slightly cheaper approximation of the old behaviour that's close-enough for a probably never-used feature. It also changes the event handling loop to not re-read the clock on every processed event. This makes the timestamp passed into the handler further behind, but it also means the inner loop handling timed events will stop earlier, and so starvation of non-timed events is less likely to occur. Signed-off-by: Erik Boasson <eb@ilities.com>
eboasson
force-pushed
the
refactor-xevent
branch
from
ec5ed36
to
529a358
Compare
dpotman
reviewed
Mar 23, 2023
There was a problem hiding this comment.
Looks good, a few comments on some minor issues. I cannot really assess what the impact is of the new approach of rounding (or actually not rounding) the schedule time and the change in reading the clock while processing timed events. Shouldn't we make the latter configurable, to avoid the issue (in certain specific scenarios) that was the reason for adding this extra update of tnow?
| @@ -49,6 +49,11 @@ struct ddsi_entity_common *ddsi_entity_common_from_proxy_endpoint_common (const | |||
| /** @component ddsi_proxy_endpoint */ | |||
| bool ddsi_is_proxy_endpoint (const struct ddsi_entity_common *e); | |||
|
|
|||
| /** @component timed_events */ | |||
There was a problem hiding this comment.
Component ddsi_proxy_endpoint?
| * @remark: cb will be called with now = NEVER if the event is still enqueued when when ddsi_xeventq_free starts cleaning up | ||
| * | ||
| * @param evq event queue | ||
| * @param tsched timestamp scheduled | ||
| * @param cb callback function pointer | ||
| * @param arg arguments for the callback | ||
| * @param arg argument (copied into ddsi_xevent) | ||
| * @param arg_size size of argument |
There was a problem hiding this comment.
sync_on_delete parameter description missing
|
|
||
| /****************************************************************************** | ||
| *** | ||
| *** SEDP |
There was a problem hiding this comment.
Given that we have a file ddsi_discovery_sedp.c, this section title is somewhat confusing (the real confusion is caused by the fact that SEDP is, despite the 'endpoint' in its name, also used for topics). Maybe we can call this file ddsi_discovery_sedp_endpoint and the other ddsi_discovery_sedp_common? And rename this section to something like 'Writing and handling SEDP for endpoints'.
|
|
||
| if ((wr = ddsi_entidx_lookup_writer_guid (gv->entity_index, &ev->u.heartbeat.wr_guid)) == NULL) | ||
| { | ||
| GVTRACE("heartbeat(wr "PGUIDFMT") writer gone\n", PGUID (ev->u.heartbeat.wr_guid)); |
There was a problem hiding this comment.
Is there a specific reason for leaving out this trace in the new xevent callback function ddsi_xevent_heartbeat_cb?
To the best of our knowledge this has not been used for at least a decade and so it is probably better to completely drop it. This commit removes the implementation but does retain the configuration option as a deprecated option so that use of it will cause a warning rather than a hard error. Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Trying to move the common parts of SEDP support into a separate file only confused things, in hindsight. This moves the contents of that file back into ddsi_discovery. Signed-off-by: Erik Boasson <eb@ilities.com>
Having "xevent" at the beginning suggests these somehow belong to the xevent handling, which they do not. The intent was to use the name to suggest these are callbacks intended to be called through the xevent mechanism. Moving the "xevent" to the end of the name seems to do a better job at this. Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 finally (almost 12 years after the introduction of a generic callback 馃槺) removes the various special event types from the timed event handling mechanism (xevents) and uses callbacks for everything. Doing so significantly reduces the set of dependencies of xevent on other components of the system and generally allows shuffling stuff around a bit. So that, too, is in this PR.
There are some other changes that kind-of happened, but that really need a good look. Those are in ec5ed36