-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
*: optimize watch stream multiplexing garbage collection #9797
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gyuho
force-pushed
the
watcher
branch
3 times, most recently
from
June 2, 2018 00:32
49bfbb1
to
530fc6e
Compare
gyuho
force-pushed
the
watcher
branch
5 times, most recently
from
June 7, 2018 21:43
9606d07
to
90e937a
Compare
$ go test -v -run XXX -bench BenchmarkWatchableStoreMultiplexWatchPut BenchmarkWatchableStoreMultiplexWatchPutSynced-4 100000 12602 ns/op 50038 B/op 11 allocs/op BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 50000 25189 ns/op 50171 B/op 11 allocs/op Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Currently, multiplexed watchers always have WatchID "0" within its watcher pool, and never getting reclaimed on cancellation. This is preparatory work for better resource utilization. Test results show that this makes synced watcher cancellation faster. And no regression on watch stream multiplexing performance. Test result 1: benchmark old ns/op new ns/op delta BenchmarkIndexCompact1-4 213 214 +0.47% BenchmarkIndexCompact100-4 13544 13496 -0.35% BenchmarkIndexCompact10000-4 732862 738492 +0.77% BenchmarkIndexCompact100000-4 23537303 25035956 +6.37% BenchmarkIndexCompact1000000-4 292751386 297354406 +1.57% BenchmarkStorePut-4 5353 5043 -5.79% BenchmarkStoreRangeKey1-4 857 863 +0.70% BenchmarkStoreRangeKey100-4 73652 73469 -0.25% BenchmarkConsistentIndex-4 3.11 3.10 -0.32% BenchmarkStorePutUpdate-4 3828 3839 +0.29% BenchmarkStoreTxnPut-4 5425 4968 -8.42% BenchmarkStoreRestoreRevs1-4 4015 4075 +1.49% BenchmarkStoreRestoreRevs10-4 7731 7753 +0.28% BenchmarkStoreRestoreRevs20-4 10940 10092 -7.75% BenchmarkWatchableStorePut-4 5385 5011 -6.95% BenchmarkWatchableStoreTxnPut-4 6695 5627 -15.95% BenchmarkWatchableStoreWatchPutSync-4 2928 2844 -2.87% BenchmarkWatchableStoreWatchPutUnsync-4 7058 9042 +28.11% BenchmarkWatchableStoreMultiplexWatchPutSynced-4 14390 15299 +6.32% BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 24573 21944 -10.70% BenchmarkWatchableStoreUnsyncedCancel-4 954 1001 +4.93% BenchmarkWatchableStoreSyncedCancel-4 0.84 0.47 -44.05% BenchmarkKVWatcherMemoryUsage-4 1548 1752 +13.18% benchmark old allocs new allocs delta BenchmarkStoreRangeKey1-4 12 12 +0.00% BenchmarkStoreRangeKey100-4 713 713 +0.00% BenchmarkConsistentIndex-4 0 0 +0.00% BenchmarkStoreTxnPut-4 15 15 +0.00% BenchmarkStoreRestoreRevs1-4 6 6 +0.00% BenchmarkStoreRestoreRevs10-4 37 37 +0.00% BenchmarkStoreRestoreRevs20-4 68 68 +0.00% BenchmarkWatchableStorePut-4 15 15 +0.00% BenchmarkWatchableStoreTxnPut-4 17 17 +0.00% BenchmarkWatchableStoreWatchPutSync-4 2 2 +0.00% BenchmarkWatchableStoreWatchPutUnsync-4 2 2 +0.00% BenchmarkWatchableStoreMultiplexWatchPutSynced-4 11 12 +9.09% BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 11 12 +9.09% BenchmarkWatchableStoreUnsyncedCancel-4 0 0 +0.00% BenchmarkWatchableStoreSyncedCancel-4 0 0 +0.00% BenchmarkKVWatcherMemoryUsage-4 8 8 +0.00% benchmark old bytes new bytes delta BenchmarkStoreRangeKey1-4 544 544 +0.00% BenchmarkStoreRangeKey100-4 40029 40029 +0.00% BenchmarkConsistentIndex-4 0 0 +0.00% BenchmarkStoreTxnPut-4 1483 1479 -0.27% BenchmarkStoreRestoreRevs1-4 586 586 +0.00% BenchmarkStoreRestoreRevs10-4 4731 4730 -0.02% BenchmarkStoreRestoreRevs20-4 9311 9311 +0.00% BenchmarkWatchableStorePut-4 1528 1539 +0.72% BenchmarkWatchableStoreTxnPut-4 1536 1533 -0.20% BenchmarkWatchableStoreWatchPutSync-4 252 251 -0.40% BenchmarkWatchableStoreWatchPutUnsync-4 272 315 +15.81% BenchmarkWatchableStoreMultiplexWatchPutSynced-4 50038 49990 -0.10% BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 50172 50133 -0.08% BenchmarkWatchableStoreUnsyncedCancel-4 0 0 +0.00% BenchmarkWatchableStoreSyncedCancel-4 0 0 +0.00% BenchmarkKVWatcherMemoryUsage-4 670 720 +7.46% Test result 2: benchmark old ns/op new ns/op delta BenchmarkIndexCompact1-4 208 207 -0.48% BenchmarkIndexCompact100-4 13776 13474 -2.19% BenchmarkIndexCompact10000-4 787606 737522 -6.36% BenchmarkIndexCompact100000-4 24437827 22737532 -6.96% BenchmarkIndexCompact1000000-4 295421494 298144196 +0.92% BenchmarkStorePut-4 5027 5357 +6.56% BenchmarkStoreRangeKey1-4 864 854 -1.16% BenchmarkStoreRangeKey100-4 73460 73495 +0.05% BenchmarkConsistentIndex-4 3.11 3.41 +9.65% BenchmarkStorePutUpdate-4 3874 3799 -1.94% BenchmarkStoreTxnPut-4 5065 5362 +5.86% BenchmarkStoreRestoreRevs1-4 4012 4019 +0.17% BenchmarkStoreRestoreRevs10-4 7915 7845 -0.88% BenchmarkStoreRestoreRevs20-4 10944 10627 -2.90% BenchmarkWatchableStorePut-4 5326 5366 +0.75% BenchmarkWatchableStoreTxnPut-4 5419 5551 +2.44% BenchmarkWatchableStoreWatchPutSync-4 2439 2755 +12.96% BenchmarkWatchableStoreWatchPutUnsync-4 9839 5919 -39.84% BenchmarkWatchableStoreMultiplexWatchPutSynced-4 13722 14325 +4.39% BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 21755 21383 -1.71% BenchmarkWatchableStoreUnsyncedCancel-4 924 1066 +15.37% BenchmarkWatchableStoreSyncedCancel-4 0.83 0.47 -43.37% BenchmarkKVWatcherMemoryUsage-4 1547 1714 +10.80% benchmark old allocs new allocs delta BenchmarkStoreRangeKey1-4 12 12 +0.00% BenchmarkStoreRangeKey100-4 713 713 +0.00% BenchmarkConsistentIndex-4 0 0 +0.00% BenchmarkStoreTxnPut-4 15 15 +0.00% BenchmarkStoreRestoreRevs1-4 6 6 +0.00% BenchmarkStoreRestoreRevs10-4 37 37 +0.00% BenchmarkStoreRestoreRevs20-4 68 68 +0.00% BenchmarkWatchableStorePut-4 15 15 +0.00% BenchmarkWatchableStoreTxnPut-4 17 17 +0.00% BenchmarkWatchableStoreWatchPutSync-4 2 2 +0.00% BenchmarkWatchableStoreWatchPutUnsync-4 2 2 +0.00% BenchmarkWatchableStoreMultiplexWatchPutSynced-4 11 12 +9.09% BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 11 12 +9.09% BenchmarkWatchableStoreUnsyncedCancel-4 0 0 +0.00% BenchmarkWatchableStoreSyncedCancel-4 0 0 +0.00% BenchmarkKVWatcherMemoryUsage-4 8 8 +0.00% benchmark old bytes new bytes delta BenchmarkStoreRangeKey1-4 544 544 +0.00% BenchmarkStoreRangeKey100-4 40029 40029 +0.00% BenchmarkConsistentIndex-4 0 0 +0.00% BenchmarkStoreTxnPut-4 1485 1476 -0.61% BenchmarkStoreRestoreRevs1-4 585 586 +0.17% BenchmarkStoreRestoreRevs10-4 4730 4731 +0.02% BenchmarkStoreRestoreRevs20-4 9311 9311 +0.00% BenchmarkWatchableStorePut-4 1528 1528 +0.00% BenchmarkWatchableStoreTxnPut-4 1557 1538 -1.22% BenchmarkWatchableStoreWatchPutSync-4 251 251 +0.00% BenchmarkWatchableStoreWatchPutUnsync-4 317 276 -12.93% BenchmarkWatchableStoreMultiplexWatchPutSynced-4 50038 49990 -0.10% BenchmarkWatchableStoreMultiplexWatchPutUnsynced-4 50183 50129 -0.11% BenchmarkWatchableStoreUnsyncedCancel-4 0 0 +0.00% BenchmarkWatchableStoreSyncedCancel-4 0 0 +0.00% BenchmarkKVWatcherMemoryUsage-4 670 720 +7.46% Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Now that watch response returns the original watch IDs, gRPC proxy test should expect coalesced watch ID, while regular test expects unique watch IDs. Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Will reopen after fixing gRPC proxy test failures. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
https://github.com/coreos/etcd/blob/d2d8c5716f92f1750ba6ef559c443a01ad4796d2/etcdserver/api/v3rpc/watch.go#L136-L150
https://github.com/coreos/etcd/blob/d2d8c5716f92f1750ba6ef559c443a01ad4796d2/mvcc/watchable_store.go#L101-L109
mvcc.(*watchableStore).NewWatchStream
creates a new&watchStream
object for every watch request, thus multiplexed watchers on one shared gRPC connection are not sharing resources between. Basically, every single watcher has watch ID 0 (unless specified), and only one single watcher in its watcher pool.Expected behavior
In the following example,
server should be creating 100 watcher objects shared under one watch stream object, each of which should be assigned a unique watch ID. However, the current master branch creates 100 separate watch streams, where all watchers have watch ID 0. There is no global state to track how many watchers have been created over one shared gRPC connection.
Having global states over a shared stream would enable reclaiming canceled watcher objects, thus better resource utilization in storage backend.
Proposal
Watch()
doesn't send&pb.WatchCancelRequest
#9416Performance
I've added more benchmarks to make sure this does not introduce any regression.
Test result 1:
Test result 2:
Test results show that this makes synced watcher cancellation faster.
And no regression on watch stream multiplexing performance.
TODO: need to handle gRPC proxy
/cc @yudai @xiang90 @jpbetz