[#1296] improvement(rust): use std RWLock to replace tokio lock for better performance #1216
Conversation
zuston
changed the title
advancedxy
left a comment
advancedxy
left a comment
There was a problem hiding this comment.
LGTM except one minor comment.
It would be great if there are some numbers about the performance improvement.
| error!("Partial purge is not supported."); | ||
| } else { | ||
| self.store.purge(app_id).await?; | ||
| self.store.purge(app_id).await? |
There was a problem hiding this comment.
this line seems unrelated?
Is it necessary?
There was a problem hiding this comment.
And the flatten dashmap will be faster than the multiple level dashmap.
This is included in description.
There was a problem hiding this comment.
oh... I mean why remove the ;. Is that related to the flatten dashmap?
There was a problem hiding this comment.
my mistake, this is not necessary.
Oh, I dig it more to improve performance, the discussion is here https://users.rust-lang.org/t/tokio-await-mutex-too-slow/98278/8 . From the maintainer of tokio mentioned, the sync lock is a good choice for us. |
|
Switch to draft state. I'm evaluating to use parking_lot crate to replace std lock |
|
cc @jerqi PTAL |
Codecov Report
@@ Coverage Diff @@
## master #1216 +/- ##
============================================
+ Coverage 53.67% 54.71% +1.04%
+ Complexity 2690 2689 -1
============================================
Files 403 383 -20
Lines 23456 21096 -2360
Branches 1992 1992
============================================
- Hits 12589 11542 -1047
+ Misses 10093 8851 -1242
+ Partials 774 703 -71 see 21 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
zuston
changed the title
4 participants
What changes were proposed in this pull request?
From the past experience, I found the sync lock performance is better than the tokio lock,
especially on the critical path.
And the flatten
dashmapwill be faster than the multiple leveldashmap.So this PR is to improve above performance point in
app.rsWhy are the changes needed?
For better performance
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UTs