Skip to content

kv: use atomic bool to avoid read lock in Replica.IsInitialized#84110

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/isInitAtomic
Jul 12, 2022
Merged

kv: use atomic bool to avoid read lock in Replica.IsInitialized#84110
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/isInitAtomic

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jul 8, 2022

According to a CPU profile acquired while running TPC-E, the read lock acquired in Replica.IsInitialized is the second most expensive read lock in terms of CPU cycles consumed. This doesn't mean that it's the most expensive in terms of blocking, but it still indicates that the lock is frequently acquired.

----------------------------------------------------------+-------------
                                             0.29s 43.28% |   github.com/cockroachdb/pebble/internal/cache.(*shard).Get github.com/cockroachdb/pebble/internal/cache/external/com_github_cockroachdb_pebble/internal/cache/clockpro.go:117 (inline)
                                             0.05s  7.46% |   github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).IsInitialized github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_init.go:249 (inline)
...
     0.67s  0.39% 24.00%      0.67s  0.39%                | sync.(*RWMutex).RLock GOROOT/src/sync/rwmutex.go:61
----------------------------------------------------------+-------------

The two main callers of this method are Store.Send (on each request) and Replica.handleRaftReadyRaftMuLocked (on each raft ready iteration):

----------------------------------------------------------+-------------
                                             0.04s 80.00% |   github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Store).Send github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/store_send.go:176
                                             0.01s 20.00% |   github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).handleRaftReadyRaftMuLocked github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_raft.go:829
     0.04s 0.023% 0.023%      0.05s 0.029%                | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).IsInitialized github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_init.go:251
                                             0.01s 20.00% |   sync.(*RWMutex).RUnlock GOROOT/src/sync/rwmutex.go:81
----------------------------------------------------------+-------------

This commit makes the method cheaper by replacing the lock with an atomic read. Once a replica is initialized, it stays initialized, so this state is well suited to be stored in an atomic value.

At a minimum, this offsets the new rlock acquired by #78980.

According to a CPU profile acquired while running TPC-E, the read lock acquired
in `Replica.IsInitialized` is the second most expensive read lock in terms of
CPU cycles consumed. This doesn't mean that it's the most expensive in terms of
blocking, but it still indicates that the lock is frequently acquired.

```
----------------------------------------------------------+-------------
                                             0.29s 43.28% |   github.com/cockroachdb/pebble/internal/cache.(*shard).Get github.com/cockroachdb/pebble/internal/cache/external/com_github_cockroachdb_pebble/internal/cache/clockpro.go:117 (inline)
                                             0.05s  7.46% |   github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).IsInitialized github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_init.go:249 (inline)
...
     0.67s  0.39% 24.00%      0.67s  0.39%                | sync.(*RWMutex).RLock GOROOT/src/sync/rwmutex.go:61
----------------------------------------------------------+-------------
```

The two main callers of this method are `Store.Send` (on each request) and
`Replica.handleRaftReadyRaftMuLocked` (on each raft ready iteration):

```
----------------------------------------------------------+-------------
                                             0.04s 80.00% |   github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Store).Send github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/store_send.go:176
                                             0.01s 20.00% |   github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).handleRaftReadyRaftMuLocked github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_raft.go:829
     0.04s 0.023% 0.023%      0.05s 0.029%                | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).IsInitialized github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_init.go:251
                                             0.01s 20.00% |   sync.(*RWMutex).RUnlock GOROOT/src/sync/rwmutex.go:81
----------------------------------------------------------+-------------
```

This commit makes the method cheaper by replacing the lock with an atomic
read. Once a replica is initialized, it stays initialized, so this state
is well suited to be stored in an atomic value.
@nvb nvb requested review from aayushshah15 and tbg July 8, 2022 21:44
@nvb nvb requested a review from a team as a code owner July 8, 2022 21:44
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jul 12, 2022

TFTRs!

bors r=knz,erikgrinaker

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 12, 2022

Build succeeded:

@craig craig bot merged commit 571bfa3 into cockroachdb:master Jul 12, 2022
@nvb nvb deleted the nvanbenschoten/isInitAtomic branch July 12, 2022 16:44
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.

4 participants