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

Improve NotificationCenter speed and usability #4414

Closed
aleks-f opened this issue Jan 24, 2024 · 4 comments
Closed

Improve NotificationCenter speed and usability #4414

aleks-f opened this issue Jan 24, 2024 · 4 comments
Assignees

Comments

@aleks-f
Copy link
Member

aleks-f commented Jan 24, 2024

Is your feature request related to a problem? Please describe.
Currently, the NotificationCenter calls (N)Observer callback in the same thread where the subscribed-to event occurs. This slows down the event collection by executing the handling functionality "inline" in the event collection loop.

Additionally, notification dispatch in (N)Observer depends on dynamic_cast to determine whether to actually post the Notification to the Observer. The downside of this approach (aside from the dynamic_cast performance hit) is that it is not possible to distinguish observers by any other criteria except the compile-time notification type.

Describe the solution you'd like
Decoupling event detection from handling would allow to collect event-generating data without being slowed down by the processing workload.
Adding an additional (runtime) matching criteria would relax the current constraints and only actually interested observers could be determined prior to dynamic casting and calling the handler.

  • NotificationCenter:
    • enqueueNotification()
  • NObserver (all optional, * is only to describe what it conceptually is, could also be a unique_ptr):
    • dequeueNotification()
    • NotificationQueue*
    • Poco::Thread*
    • Dispatcher* (Runnable child)
    • (*match)(const Poco::Any&) callback

Describe alternatives you've considered
Alternatively, this could be implemented as a separate pair of classes, inheriting from the existing ones (eg. ActiveNotificationCenter and ActiveObserver)

Additional context
All this should be added as a default-off option, ie. without affecting the existing functionality.
I'm inclined to deprecate the Poco::Observer in the future, eventually renaming NObserver to Observer with perhaps notifications being std::unique_ptr which are moved into the handlers.

@aleks-f aleks-f changed the title Improve NotificationCenter speed Improve NotificationCenter speed and usability Jan 24, 2024
@aleks-f aleks-f added this to the Release 1.14.0 milestone Jan 24, 2024
@aleks-f aleks-f added this to 1.14 Jan 24, 2024
@matejk
Copy link
Contributor

matejk commented Jan 24, 2024

NObserver and Observer shall definitely be merged into one implementation.

Is NotificationCenter::enqueueNotification() planned to be an enhanced postNotification() which does criteria matching?

Will a new thread be introduced that performs notification dispatching to registered observers to avoid the "inline" handling function?

@obiltschnig
Copy link
Member

Rather than one class making do too much, I'd prefer a new class that does things differently, but in one specific way only.

@aleks-f
Copy link
Member Author

aleks-f commented Jan 25, 2024

Is NotificationCenter::enqueueNotification() planned to be an enhanced postNotification() which does criteria matching?

No, NotificationCenter should only enqueue notifications into a NotificationQueue.

Will a new thread be introduced that performs notification dispatching to registered observers to avoid the "inline" handling function?

Yes, there should be a "dequeuer", running in a thread, dequeuing notifications and notifying observers. Every observer should have two function pointers: bool match() and handle(), the latter called only if the former returns true.

Matching could be done through an Any parameter, or maybe it's better if we simply use the Notification::name() string.

The goals are to (1) decouple event posting from handling and (2) avoid notifying non-interested observers (currently, there is no way to indicate interest in just a subset of notifications of certain type - every subscriber to a Notification subtype gets all the notifications of that type, plus there's a dynamic_cast performance hit in Observer.

@aleks-f
Copy link
Member Author

aleks-f commented Feb 6, 2024

I made AsyncObserver child of NObserver. NotificationCenter remained (almost) intact.

aleks-f added a commit that referenced this issue Feb 16, 2024
* feat(AsyncObserver): Improve NotificationCenter speed and usability #4414

* fix(Notification): add missing header

* feat(Any): add checkers for holding nullptr #4447

* feat(NotificationCenter): g++ build and refactoring #4414

* fix(Observer): compile errors on some compilers #4414

* fix(NotificationCenter): compile errors #4414

* chore(ParallelSocketAcceptor): remove unnecessary include and using from header

* feat(AsyncNotificationCenter): add #4414

* test(AsyncNotificationCenter): add mixed observer types to the test #4414

* fix(AsyncNotificationCenter): hangs on program exit #4414

* fix(dev): friend not honored, temporarily make private members public

* fix(AsyncNotificationCenter); remove default #4414
@aleks-f aleks-f added the fixed label May 13, 2024
@aleks-f aleks-f self-assigned this May 13, 2024
@aleks-f aleks-f moved this to Done in 1.14 May 13, 2024
@matejk matejk closed this as completed Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

4 participants