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

Potentially replace locks to improve performance #1

Open
Net5F opened this issue Sep 24, 2021 · 1 comment
Open

Potentially replace locks to improve performance #1

Net5F opened this issue Sep 24, 2021 · 1 comment
Labels
enhancement New feature or request

Comments

@Net5F
Copy link
Owner

Net5F commented Sep 24, 2021

notify(), subscribe(), and unsubscribe() currently all acquire a scoped_lock before accessing/modifying queueVectorMap. This is to maintain thread safety in case someone tries to sub/unsub and notify at the same time. (Note that getQueueVectorForType() also potentially modifies the map.)

If we remove the locks, notify() will potentially get a performance boost. In my not-great test at the bottom of TestQueuedEvents.cpp, it goes from 2x the overhead of the raw ReaderWriteQueue to just 1.4x.

I can see a scenario where we sacrifice the ability to notify & sub/unsub at the same time in favor of a faster notify (people can synchronize it themselves if they care to). This is justifiable since most use cases probably involve constructing all of your queues once at the start of your application, and then only ever notifying.

If we do this, we would have to find an efficient way to error if someone does try to call notify & sub/unsub at the same time (maybe only enable it in debug? that might be fine). We would also have to solve the case of destruction at the end of the application's life cycle (producer threads may try to notify while the consumer is already destructing).

@Net5F Net5F added the enhancement New feature or request label Sep 24, 2021
@Net5F
Copy link
Owner Author

Net5F commented Sep 27, 2021

Performance is closer to a raw queue after switching the queued type to a non-shared_ptr. This shows that the current minimal testing isn't very useful. Would need better testing to make a decision here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant