Skip to content

Removing need to use Mutex on ScheduledTimer's slot field #28

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

Closed
wants to merge 11 commits into from
Closed

Removing need to use Mutex on ScheduledTimer's slot field #28

wants to merge 11 commits into from

Conversation

Chopinsky
Copy link

Removing the need to use Mutex on on ScheduledTimer's slot field

Description

After analyzing the usage of ScheduledTimer's slot field in the library, it is determined that it is possible to remove Mutex protection from ScheduledTimer's slot field, and replace it with a more
generic and light-weight AtomicUsize struct.

A few code changes are made to accommodate the removal of the lock code and using its replacement of the atomic structure.

Motivation and Context

It has been documented as a TODO in the library source code that use Mutex for the slot field is probably an overkill and we should be able to explore an alternative solution for the same purpose. After investigations, it's clear that such goal is achievable, since modifying a ScheduleTimer instance almost always involves acquiring a Mutex lock for the at field, that effectively fend off all concurrent access or modifications on the instance, hence guarantee the thread safety.

In specific, all code that access or update the slot field are either called from timer's poll method, or from the reset code (i.e. from Timer's advance_to method). The former's thread-safety is guaranteed the by locking the entire node via the at-field's lock structure, hence the update_or_add and remove call are essentially synchronized code; the later is a bit complex, though the intention is to clean up the slot, field, because the index location it points to has already been vacated from the previous pop call, hence we shall simply reset the slot index to 0 at this point.

How Has This Been Tested?

  • Integrated test and unit tests have been run.
  • End to end tests on timeout-related features have been tested as well.

Types of changes

  • [ X ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ ] My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [ ] I have read the CONTRIBUTING document.
  • I have added tests to cover my changes (existing tests already cover all the affected code area).
  • All new and existing tests passed.

@taiki-e taiki-e mentioned this pull request Aug 28, 2019
8 tasks
@spacejam
Copy link

spacejam commented Sep 2, 2019

Real quick before looking deeper, is there any measurable performance change by avoiding the mutex? In general, lock-free stuff should only be considered once there's a clear justification for the risk

@Chopinsky
Copy link
Author

Chopinsky commented Sep 4, 2019

Real quick before looking deeper, is there any measurable performance change by avoiding the mutex? In general, lock-free stuff should only be considered once there's a clear justification for the risk

I don't have the bench, since the affected functions and structs are all private, and measuring the performance of the timer itself is not quite straightforward, since there could be random delays depending on the timing when it's polled. But I'm open to proposals on how to make a bench for the changes.

Also, this PR is not quite the same as "making everything lock-free", here we just tried to eliminate a mutex protection as it's not actually needed -- if I'm reading the code correctly, a lock on another field of the same struct (the at field) would have already granted the exclusive write/read access to the underlying Slot object for the affected code path, hence we can simply replace the mutex locks with atomic structures. But please do correct me if we've made a wrong assertions here.

@yoshuawuyts yoshuawuyts mentioned this pull request Oct 9, 2019
@yoshuawuyts
Copy link
Contributor

I don't think this PR is worth it. Thanks heaps!

@yoshuawuyts yoshuawuyts closed this Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants