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

feat: use ConcurrentMap to replace map and mutex #187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RealBar
Copy link

@RealBar RealBar commented Sep 30, 2020

Compared to map + sync.Mutex, ConcurrentMap is proved reliable and more efficient

@xtaci
Copy link
Owner

xtaci commented Sep 30, 2020

how is this compared with sync.Map?

@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #187 into master will decrease coverage by 1.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   81.89%   80.51%   -1.39%     
==========================================
  Files          11       11              
  Lines        2055     1863     -192     
==========================================
- Hits         1683     1500     -183     
+ Misses        288      280       -8     
+ Partials       84       83       -1     
Impacted Files Coverage Δ
sess.go 81.71% <100.00%> (-1.16%) ⬇️
tx.go 70.00% <0.00%> (-5.00%) ⬇️
readloop_linux.go 65.38% <0.00%> (-4.62%) ⬇️
readloop.go 70.83% <0.00%> (-4.17%) ⬇️
tx_linux.go 75.00% <0.00%> (-3.27%) ⬇️
crypt.go 75.74% <0.00%> (-1.54%) ⬇️
timedsched.go 89.70% <0.00%> (-1.44%) ⬇️
fec.go 88.02% <0.00%> (-1.35%) ⬇️
kcp.go 77.61% <0.00%> (-1.19%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f45217e...0b6888f. Read the comment docs.

@xtaci
Copy link
Owner

xtaci commented Sep 30, 2020

if there's only one link, the benefit from your algorithm is only the xxhash algorithm right? the locking part is still the RLock?

How about caching + sync.Map?

@xtaci
Copy link
Owner

xtaci commented Sep 30, 2020

608bd97

Change-Id: I923ef54bcd475a460af6eadc681aeeff5592f335
@RealBar
Copy link
Author

RealBar commented Sep 30, 2020

segment lock also helps a lot

@RealBar
Copy link
Author

RealBar commented Sep 30, 2020

benchmark is

BenchmarkConcurrentMap_Write-8 4402040 344 ns/op
BenchmarkSyncMap_Write-8 1000000 1869 ns/op
BenchmarkConcurrentMap_ReadWrite-8 398430 2947 ns/op
BenchmarkSyncMap_ReadWrite-8 365456 3240 ns/op
BenchmarkMapMutex_ReadWrite-8 336242 3683 ns/op

@xtaci
Copy link
Owner

xtaci commented Sep 30, 2020

For random r/w, segment lock is efficient surely.

But in kcp-go, for most of the time, just read lock is required to retrieve the session object.

Write lock only happens twice(1. connection establishment, 2. connection termination)

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.

None yet

2 participants