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

Replace pthread_rwlock with std::shared_mutex #10216

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

rgacogne
Copy link
Member

Short description

Closes #10209.

The speed tests result looks good, except perhaps for a small regression in the "RW lock shared" case but since we are talking of 1 usec for 1000 operations, I believe we can live with that.

  • master
'getlock-uncontended-test' 0.11 seconds: 112247847.3 runs/s, 0.01 usec/run
'RW lock shared' 0.11 seconds: 107827.9 runs/s, 9.27 usec/run
'RW lock exclusive' 0.11 seconds: 72534.4 runs/s, 13.79 usec/run
'RW lock try exclusive - non-contended' 0.11 seconds: 69176.3 runs/s, 14.46 usec/run
'RW lock try exclusive - contended' 0.11 seconds: 255326.8 runs/s, 3.92 usec/run
'RW lock try shared - non-contended' 0.11 seconds: 107123.5 runs/s, 9.34 usec/run
'RW lock try shared - contended' 0.11 seconds: 255821.4 runs/s, 3.91 usec/run

'getlock-uncontended-test' 0.11 seconds: 113262655.2 runs/s, 0.01 usec/run
'RW lock shared' 0.11 seconds: 108756.1 runs/s, 9.19 usec/run
'RW lock exclusive' 0.11 seconds: 73071.9 runs/s, 13.69 usec/run
'RW lock try exclusive - non-contended' 0.11 seconds: 69429.8 runs/s, 14.40 usec/run
'RW lock try exclusive - contended' 0.11 seconds: 257484.5 runs/s, 3.88 usec/run
'RW lock try shared - non-contended' 0.11 seconds: 108059.3 runs/s, 9.25 usec/run
'RW lock try shared - contended' 0.11 seconds: 257798.9 runs/s, 3.88 usec/run
  • PR
'getlock-uncontended-test' 0.11 seconds: 111842329.5 runs/s, 0.01 usec/run
'RW lock shared' 0.11 seconds: 95161.0 runs/s, 10.51 usec/run
'RW lock exclusive' 0.11 seconds: 72660.4 runs/s, 13.76 usec/run
'RW lock try exclusive - non-contended' 0.11 seconds: 71733.0 runs/s, 13.94 usec/run
'RW lock try exclusive - contended' 0.11 seconds: 360459.7 runs/s, 2.77 usec/run
'RW lock try shared - non-contended' 0.11 seconds: 106830.7 runs/s, 9.36 usec/run
'RW lock try shared - contended' 0.11 seconds: 255051.5 runs/s, 3.92 usec/run

'getlock-uncontended-test' 0.11 seconds: 112064700.8 runs/s, 0.01 usec/run
'RW lock shared' 0.11 seconds: 96040.2 runs/s, 10.41 usec/run
'RW lock exclusive' 0.11 seconds: 72596.2 runs/s, 13.77 usec/run
'RW lock try exclusive - non-contended' 0.11 seconds: 71517.1 runs/s, 13.98 usec/run
'RW lock try exclusive - contended' 0.11 seconds: 360546.4 runs/s, 2.77 usec/run
'RW lock try shared - non-contended' 0.11 seconds: 107134.1 runs/s, 9.33 usec/run
'RW lock try shared - contended' 0.11 seconds: 255380.6 runs/s, 3.92 usec/run

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

pdns/speedtest.cc Outdated Show resolved Hide resolved
pdns/speedtest.cc Outdated Show resolved Hide resolved
pdns/speedtest.cc Outdated Show resolved Hide resolved
Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

What I always found confusing: to declare a lock object you use ReadWriteLock, but to take a lock is ReadLock or WriteLock. I'd love to rename the latter to TakeReadLock and TakeWriteLock. Or maybe Acquire... ?

Alternatively we could rename ReadWriteLock to ReadWriteLockObject.

@rgacogne rgacogne closed this Mar 26, 2021
@rgacogne rgacogne deleted the shared-lock branch March 26, 2021 10:57
@rgacogne
Copy link
Member Author

That feels like a different pull request to me but sure, feel free to do so.

@rgacogne rgacogne restored the shared-lock branch March 26, 2021 11:17
@rgacogne rgacogne reopened this Mar 26, 2021
@omoerbeek
Copy link
Member

omoerbeek commented Mar 26, 2021

That feels like a different pull request to me but sure, feel free to do so.

Yes, that would be a change in interface while this PR only concerns implementation. I agree a separate PR is better for that part.

@omoerbeek
Copy link
Member

I'll test soon to see if linux/clang has the same performance regression as OpenBSD/clang (about 2x slower).

@rgacogne
Copy link
Member Author

What I always found confusing: to declare a lock object you use ReadWriteLock, but to take a lock is ReadLock or WriteLock. I'd love to rename the latter to TakeReadLock and TakeWriteLock. Or maybe Acquire... ?

Perhaps ReadLockHolder and WriteLockHolder?

@omoerbeek
Copy link
Member

I think it is clear OpenBSD is the outlier.

On buster using clang-7 using a random laptop with an Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz

master lock.hh

'getlock-uncontended-test' 0.10 seconds: 46332207.3 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 40933.0 runs/s, 24.43 usec/run
'RW lock exclusive' 0.10 seconds: 31021.2 runs/s, 32.24 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 35554.8 runs/s, 28.13 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 156333.8 runs/s, 6.40 usec/run
'RW lock try shared - non-contended' 0.11 seconds: 45011.4 runs/s, 22.22 usec/run
'RW lock try shared - contended' 0.10 seconds: 149328.7 runs/s, 6.70 usec/run

'getlock-uncontended-test' 0.10 seconds: 46323242.3 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 40940.3 runs/s, 24.43 usec/run
'RW lock exclusive' 0.10 seconds: 31007.2 runs/s, 32.25 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 35562.7 runs/s, 28.12 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 156237.3 runs/s, 6.40 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45044.9 runs/s, 22.20 usec/run
'RW lock try shared - contended' 0.10 seconds: 149309.6 runs/s, 6.70 usec/run

PR:

'getlock-uncontended-test' 0.10 seconds: 46330696.6 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43152.7 runs/s, 23.17 usec/run
'RW lock exclusive' 0.10 seconds: 36010.2 runs/s, 27.77 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36040.1 runs/s, 27.75 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252381.8 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45053.8 runs/s, 22.20 usec/run
'RW lock try shared - contended' 0.10 seconds: 205280.7 runs/s, 4.87 usec/run

'getlock-uncontended-test' 0.10 seconds: 46330866.0 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43131.5 runs/s, 23.18 usec/run
'RW lock exclusive' 0.10 seconds: 36077.9 runs/s, 27.72 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36063.9 runs/s, 27.73 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252517.0 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45043.7 runs/s, 22.20 usec/run
'RW lock try shared - contended' 0.10 seconds: 205220.1 runs/s, 4.87 usec/run

So here the PR is better. To compare, I also checked with clang-8:
master:

'getlock-uncontended-test' 0.10 seconds: 46332160.1 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 40929.9 runs/s, 24.43 usec/run
'RW lock exclusive' 0.10 seconds: 31201.5 runs/s, 32.05 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 35547.9 runs/s, 28.13 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 149345.5 runs/s, 6.70 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45055.2 runs/s, 22.19 usec/run
'RW lock try shared - contended' 0.10 seconds: 149340.9 runs/s, 6.70 usec/run

'getlock-uncontended-test' 0.10 seconds: 46294863.5 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 40949.5 runs/s, 24.42 usec/run
'RW lock exclusive' 0.10 seconds: 31019.1 runs/s, 32.24 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 35525.7 runs/s, 28.15 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 149294.1 runs/s, 6.70 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45048.0 runs/s, 22.20 usec/run
'RW lock try shared - contended' 0.10 seconds: 149315.0 runs/s, 6.70 usec/run

PR:

'getlock-uncontended-test' 0.10 seconds: 46322617.8 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43129.5 runs/s, 23.19 usec/run
'RW lock exclusive' 0.10 seconds: 35687.1 runs/s, 28.02 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 35986.0 runs/s, 27.79 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252474.4 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45034.9 runs/s, 22.20 usec/run
'RW lock try shared - contended' 0.10 seconds: 193297.2 runs/s, 5.17 usec/run

'getlock-uncontended-test' 0.10 seconds: 46318164.1 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43153.5 runs/s, 23.17 usec/run
'RW lock exclusive' 0.10 seconds: 35674.4 runs/s, 28.03 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36030.9 runs/s, 27.75 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252454.5 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45055.3 runs/s, 22.19 usec/run
'RW lock try shared - contended' 0.10 seconds: 193222.4 runs/s, 5.18 usec/run

And gcc8.3:

master:

'gettimeofday-test' 0.10 seconds: 51933786.5 runs/s, 0.02 usec/run
'getlock-uncontended-test' 0.10 seconds: 47145391.3 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43228.0 runs/s, 23.13 usec/run
'RW lock exclusive' 0.10 seconds: 35441.5 runs/s, 28.22 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36027.0 runs/s, 27.76 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252328.7 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45056.3 runs/s, 22.19 usec/run
'RW lock try shared - contended' 0.10 seconds: 234651.4 runs/s, 4.26 usec/run

'getlock-uncontended-test' 0.10 seconds: 47141655.2 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43223.8 runs/s, 23.14 usec/run
'RW lock exclusive' 0.10 seconds: 35431.7 runs/s, 28.22 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36049.2 runs/s, 27.74 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252673.9 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45064.6 runs/s, 22.19 usec/run
'RW lock try shared - contended' 0.10 seconds: 234650.2 runs/s, 4.26 usec/run

PR:

'getlock-uncontended-test' 0.10 seconds: 47131020.4 runs/s, 0.02 usec/run
'RW lock shared' 0.10 seconds: 43212.5 runs/s, 23.14 usec/run
'RW lock exclusive' 0.10 seconds: 36152.2 runs/s, 27.66 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36080.3 runs/s, 27.72 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252531.8 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45069.4 runs/s, 22.19 usec/run
'RW lock try shared - contended' 0.10 seconds: 252239.7 runs/s, 3.96 usec/run

'RW lock shared' 0.10 seconds: 43159.2 runs/s, 23.17 usec/run
'RW lock exclusive' 0.10 seconds: 35734.9 runs/s, 27.98 usec/run
'RW lock try exclusive - non-contended' 0.10 seconds: 36079.4 runs/s, 27.72 usec/run
'RW lock try exclusive - contended' 0.10 seconds: 252365.9 runs/s, 3.96 usec/run
'RW lock try shared - non-contended' 0.10 seconds: 45016.8 runs/s, 22.21 usec/run
'RW lock try shared - contended' 0.10 seconds: 252562.7 runs/s, 3.96 usec/run

@rgacogne
Copy link
Member Author

I'm merging this PR, we will rename the ReadLock and WriteLock in a different one.

@rgacogne rgacogne merged commit ecdd26e into PowerDNS:master Mar 31, 2021
@rgacogne rgacogne deleted the shared-lock branch March 31, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ponder replacing pthread_rwlock with std::shared_mutex
2 participants