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

Performance modify rate: contention on syntax (oid/name) RWlocks #4598

Open
tbordaz opened this issue Feb 5, 2021 · 5 comments
Open

Performance modify rate: contention on syntax (oid/name) RWlocks #4598

tbordaz opened this issue Feb 5, 2021 · 5 comments
Labels
performance Issue impacts performance
Milestone

Comments

@tbordaz
Copy link
Contributor

tbordaz commented Feb 5, 2021

Issue Description
/opt/ldapbench/bin/modrate --hostname instance --port 389 --bindDN 'cn=Directory Manager' --bindPassword xxx --entryDN id=user.[1-10000],ou=People,dc=example,dc=com" --attribute street --attribute description --valueLength 12 --numThreads 5

updates two non unindexed attributes. No need to put more threads as updates are serialized it only increases response time.

By default the syntax names/oids tables are protected by a lock (each one). These locks are the one on higher contention (21%). Attribute syntax are lookup for normalization/comparison.

#        ----------- Cacheline ----------    Total      Tot  ----- LLC Load Hitm -----  ---- Store Reference ----
# Index             Address  Node  PA cnt  records     Hitm    Total      Lcl      Rmt    Total    L1Hit   L1Miss
# .....  ..................  ....  ......  .......  .......  .......  .......  .......  .......  .......  .......
#
      0      0x7fd799028500     0     290     3204   21.07%       59       59        0     1435     1435        0
      1      0x7fd79902a980     0      95      139    3.57%       10       10        0       41       41        0
      2      0x7fd799028340     0      52      105    2.50%        7        7        0       38       38        0
      3      0x7fd7868e1000     0     140      272    2.14%        6        6        0      168      162        6
...

When turning off the protection 'nsslapd-schemamod: off' it gives a 5% gain of throughput (similar resp time): 3555/s vs 3370/s.
This issue is to evaluate possible improvements.

Package Version and Platform:
All plateforms/versions

Steps to Reproduce
provision a 10K entries and apply updates MOD_REPLACE on non indexed attributes

Expected results
Reduce the contention.

@tbordaz tbordaz added the needs triage The issue will be triaged during scrum label Feb 5, 2021
@mreynolds389
Copy link
Contributor

Something I had a patch for and lost :-(, was having each worker thread have its own copy of these tables. The only issue was how to deal with refreshing those per thread copies without it being too costly. Anyway, I just wanted to present this idea for this issue.

@Firstyear
Copy link
Contributor

@mreynolds389 If I make a C ffi wrapper to https://docs.rs/concread/0.2.7/concread/cowcell/struct.CowCell.html this would be perfect. It allows rwlock semantics but with no blocking on writes.

@tbordaz
Copy link
Contributor Author

tbordaz commented Feb 8, 2021

@Firstyear, there is no schema update (writers) in the tests. I guess the contention appears with the way the underlying RWlock is implemented where an 'internal' lock is acquired to update the RWlock structure.
@mreynolds389, I like the idea of a struct per thread. However if the struct must be updated (schema update) shouldn't it protected by a kind of lock (if for example we are using condvar).

@Firstyear
Copy link
Contributor

Yes, you need to lock/atomic to get a read lock which is expensive.

The cowcell I showed works that when you "read" you get a transaction to that item, and if someone writes they have to copy and then write the content. It's kind of exactly what you want in this case .... It's a rwlock without any blocking when a writer comes in. And internally it uses pthread mutex so it's going to be faster than nspr.

@tbordaz
Copy link
Contributor Author

tbordaz commented Feb 9, 2021

@Firstyear, sorry I miss how cowcell helps in this case.
In our case there are only readers and the contention appears because of the cost of underlying lock/atomic to acquire the read lock.
With cowcell, writers are not part of the game as there are no writer. Each readers have to get a transaction which looks to me pretty similar to underlying lock/atomic to acquire the read lock. Does it means the cost of the transaction is less expensive than the lock/atomic ?

Note that at the moment syntax RWlock are using pthread rwlock.

I understand the need to protect name/oid tables. But changes are pretty rare (even with replication schema update is not that frequent) and it is pretty frustration to see this contention for rare events.
With @mreynolds389 approach, we could imagine a local (per thread) copy of the tables (not protected). The local copy can get out of date from reference (that is protected by a lock). periodically, the thread will resync its local copy from the reference.

@mreynolds389 mreynolds389 removed the needs triage The issue will be triaged during scrum label Feb 11, 2021
@mreynolds389 mreynolds389 added this to the 2.0.0 milestone Feb 11, 2021
@mreynolds389 mreynolds389 added the performance Issue impacts performance label Jan 12, 2022
mreynolds389 added a commit to mreynolds389/389-ds-base that referenced this issue Apr 5, 2023
Description:

There is a lot of contention around the rwlocks for the attribute
syntax hashtables.  Since syntaxes rarely change we can just keep a
copy in each thread and avoid locking.

Then the worker can check for changes on each loop and rebuild
the hashtables as needed.

Did some code cleanup in schema.c

relates: 389ds#4598

Reviewed by: progier(Thanks!)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issue impacts performance
Projects
None yet
Development

No branches or pull requests

3 participants