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

Remove unnecessary locks in keeperStore #238

Merged

Conversation

lzydmxy
Copy link
Contributor

@lzydmxy lzydmxy commented Apr 25, 2024

Which issues of this PR fixes:

This PR try to fix #239

Change log:

  • remove node's lock
  • remove nodeMap's lock

Slightly peformance improve

# before
thread_size,tps,avgRT(microsecond),TP90(microsecond),TP99(microsecond),TP999(microsecond),failRate
200,97658,2064.0,2900.0,3700.0,5200.0,0.0
200,97157,2066.0,2900.0,3800.0,6000.0,0.0
# after
thread_size,tps,avgRT(microsecond),TP90(microsecond),TP99(microsecond),TP999(microsecond),failRate
200,100083,2014.0,3000.0,3800.0,5500.0,0.0
200,99541,2025.0,3000.0,3700.0,5200.0,0.0

@lzydmxy lzydmxy changed the title Remove unnecessary locks keeper store Remove unnecessary locks in keeperStore Apr 25, 2024
@JackyWoo JackyWoo added the performance Performance promotion label Apr 25, 2024
@JackyWoo JackyWoo added this to the Release v2.0.5 milestone Apr 25, 2024
@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 25, 2024

@JackyWoo Failed for TEST(RaftPerformance, machineCreateThread), should i remove it ? Since RK::ConcurrentMap is no longer thread-safe

@JackyWoo
Copy link
Contributor

JackyWoo commented Apr 25, 2024

We can make it single thread. Also let's rename ConcurrentMap .

@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 25, 2024

We can make it single thread. Also let's rename ConcurrentMap .

okay

@JackyWoo
Copy link
Contributor

@lzydmxy let's test the performance with relaxed memory order of node_count which should provide some promotion.

@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 28, 2024

Snapshot load faster than before. 49s vs 22s

# before
2024.04.28 12:19:26.121504 [ 2481991 ] <Information> KeeperSnapshotStore: Parse object from disk
2024.04.28 12:20:12.085832 [ 2481991 ] <Information> KeeperSnapshotStore: Build ChildrenSet for nodes
2024.04.28 12:20:15.807878 [ 2481991 ] <Information> KeeperSnapshotStore: Load snapshot done: nodes 58659723, ephemeral nodes 0, sessions 0, session_id_counter 121406, zxid 52981993478
#after
2024.04.28 12:26:09.077839 [ 2488186 ] <Information> KeeperSnapshotStore: Parse object from disk
2024.04.28 12:26:20.132151 [ 2488186 ] <Information> KeeperSnapshotStore: Build DataTree
2024.04.28 12:26:31.131679 [ 2488186 ] <Information> KeeperSnapshotStore: Load snapshot done: nodes 58659723, ephemeral nodes 0, sessions 0, session_id_counter 121406, zxid 52981993478

This is the impact of recent optimizations on the loading speed of snapshots.
Snapshot with 58659723 Nodes
Native(snapshot v1) 160s
After #222 99s
After #228 (snapshot v3) 49s

@JackyWoo JackyWoo merged commit c350592 into JDRaftKeeper:master May 6, 2024
7 checks passed
@JackyWoo JackyWoo added code refactor Code refactor performance Performance promotion and removed performance Performance promotion code refactor Code refactor labels Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance promotion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary locks in KeeperStore
2 participants