region cache: batch find regions by key ranges from cache #1410

wshwsh12 · 2024-07-31T09:12:35Z

Currently, client-go only supports batch reading of region information from pd, but not from region cache.
If there is a large table with 200k regions, even if all regions are in memory, it still takes 0.8-0.9s to read all regions.
This PR optimizes the querying times to the B-tree, the time consumption reduces by about 40%.

This PR
Running tool: /gosdk/go1.22.1/bin/go test -benchmem -run=^$ -bench ^BenchmarkBatchLocateKeyRanges$ github.com/tikv/client-go/v2/internal/locate --tags=intest --count=1 -v

goos: linux
goarch: amd64
pkg: github.com/tikv/client-go/v2/internal/locate
cpu: 13th Gen Intel(R) Core(TM) i9-13900KF
BenchmarkBatchLocateKeyRanges
BenchmarkBatchLocateKeyRanges-32    	    1116	   1305628 ns/op	 1412405 B/op	   10921 allocs/op
PASS
ok  	github.com/tikv/client-go/v2/internal/locate	1.987s

Running tool: /gosdk/go1.22.1/bin/go test -benchmem -run=^$ -bench ^BenchmarkBatchLocateKeyRanges$ github.com/tikv/client-go/v2/internal/locate --tags=intest --count=1 -v

Master Branch
goos: linux
goarch: amd64
pkg: github.com/tikv/client-go/v2/internal/locate
cpu: 13th Gen Intel(R) Core(TM) i9-13900KF
BenchmarkBatchLocateKeyRanges
BenchmarkBatchLocateKeyRanges-32    	     502	   2367271 ns/op	 1512372 B/op	   20020 allocs/op
PASS
ok  	github.com/tikv/client-go/v2/internal/locate	1.831s

internal/locate/region_cache.go

you06 · 2024-08-02T07:57:19Z

internal/locate/sorted_btree.go

+	f := func(item *btreeItem) bool {
+		region := item.cachedRegion
+		if len(startKey) > 0 && bytes.Compare(region.StartKey(), startKey) <= 0 {
+			if !region.checkRegionCacheTTL(time.Now().Unix()) {


Move time.Now().Unix() out of the check loop?

you06 · 2024-08-02T07:59:54Z

internal/locate/region_cache.go

 	var regions []*Region
 	c.mu.RLock()
 	defer c.mu.RUnlock()
-	regions = c.mu.sorted.AscendGreaterOrEqual(startKey, endKey, limit)
+	regions = c.mu.sorted.DescendLessOrEqual(startKey, endKey)
+	slices.Reverse(regions)


Why not just use ascend scan instead of using descend scan and revert it.

just use ascend scan maybe lost the first region. For example, search range [b,d) from region [a, c) [c, e), use ascend scan can only get region [c, e).

you06 · 2024-08-02T13:30:35Z

internal/locate/sorted_btree.go

+			regions = append(regions, region)
+			return false
+		}
+		if !region.checkRegionCacheTTL(time.Now().Unix()) {


There is also a now~

you06 · 2024-08-02T13:45:57Z

internal/locate/sorted_btree.go

+}
+
+// DescendLess returns all items that are less than or equal to the key.
+func (s *SortedRegions) DescendLessOrEqual(startKey, endKey []byte) (regions []*Region) {


Another question, why or-equal for this location func? IMO, endKey should be excluded bound.

BTree doesn't provide the function DescendLess...

I'm a little concened here that this function holds a global lock, if it takes too long time, maybe this sql will affect the other sqls, especially the oltp queries.

windtalker · 2024-08-06T01:21:35Z

internal/locate/region_cache.go

+		}
+		hit := false
+		for _, r := range batchRegionInCache {
+			if !r.Contains(keyRange.StartKey) { // uncached hole, load the rest regions


I don't understand this code. If [keyRange.StartKey, keyRange.Endkey) contains multiple regions, in my understanding only the first region can contain keyRange.StartKey?

After processing a region, I set keyRange.StartKey = r.endkey(). So the next region also can contain the keyRange.StartKey.

internal/locate/region_cache.go

windtalker · 2024-08-06T01:39:00Z

internal/locate/sorted_btree.go

+}
+
+// DescendLess returns all items that are less than or equal to the key.
+func (s *SortedRegions) DescendLessOrEqual(startKey, endKey []byte) (regions []*Region) {


I'm a little concened here that this function holds a global lock, if it takes too long time, maybe this sql will affect the other sqls, especially the oltp queries.

add benchmark Signed-off-by: wshwsh12 <[email protected]>

Signed-off-by: wshwsh12 <[email protected]>

windtalker · 2024-08-06T08:43:32Z

internal/locate/region_cache.go

+		}
+		keyRange.StartKey = r.EndKey()
+		// Batch load rest regions from Cache.
+		batchSize := 100


I think you can use defaultRegionsPerBatch as the batchSize?

windtalker · 2024-08-06T08:43:55Z

internal/locate/region_cache.go

+				keyRange.StartKey = r.EndKey()
+			}
+			if len(batchRegionInCache) < batchSize { // region cache miss, load the rest regions
+				break outer


looks like just use break here is ok

windtalker · 2024-08-06T08:47:03Z

internal/locate/region_cache.go

@@ -2078,7 +2105,8 @@ func (c *RegionCache) loadRegionByID(bo *retry.Backoffer, regionID uint64) (*Reg
 	}
 }

-// TODO(youjiali1995): for optimizing BatchLoadRegionsWithKeyRange, not used now.
+// For optimizing BatchLocateKeyRanges, scanRegionsFromCache scans at most `limit` regions from cache.
+// The first region's startKey is `startKey`, all regions returned are continuous and have no hole.


maybe you need to emphasize that it is the caller's responsibility to make sure that startKey is a node in the B-tree, otherwise, the startKey will not be included in the return regions?

Signed-off-by: wshwsh12 <[email protected]>

windtalker · 2024-08-06T12:46:53Z

internal/locate/sorted_btree.go

+		if !region.checkRegionCacheTTL(now) {
+			return false
+		}
+		if len(lastStartKey) > 0 && !region.Contains(lastStartKey) { // uncached hole


when will len(lastStartKey) == 0?

Some test cases have the key, for -inf to +inf.

The case len(lastStartKey) == 0 should never happen in current usage(start key is assigned here, and it's never empty if the code runs here).

When len(lastStartKey) == 0 and the first region is not start from -inf, we should stop scan regions, see the following test.

func (s *testRegionRequestToSingleStoreSuite) TestRegionCacheStartNonEmpty() { _ = s.cache.refreshRegionIndex(s.bo) r, _ := s.cache.scanRegionsFromCache(s.bo, []byte{}, nil, 10) s.Equal(len(r), 1) region, _ := s.cache.LocateRegionByID(s.bo, s.region) v2 := region.Region.confVer + 1 r2 := metapb.Region{Id: region.Region.id, RegionEpoch: &metapb.RegionEpoch{Version: region.Region.ver, ConfVer: v2}, StartKey: []byte{1}} st := newUninitializedStore(s.store) s.cache.mu.Lock() s.cache.mu.sorted.Clear() s.cache.mu.Unlock() // region cache after clear: [] s.cache.insertRegionToCache(&Region{meta: &r2, store: unsafe.Pointer(st), ttl: nextTTLWithoutJitter(time.Now().Unix())}, true, true) // region cache after insert: [[1, +inf)] r, _ = s.cache.scanRegionsFromCache(s.bo, []byte{}, nil, 10) s.Equal(len(r), 0) // fail in current implementation }

I add the test TestRegionCacheStartNonEmpty and remove the len(lastStartKey) == 0 check. Now the code stop scan regions when len(lastStartKey) == 0 and the first region is not start from -inf. PTAL again.

Signed-off-by: wshwsh12 <[email protected]>

windtalker · 2024-08-07T10:06:34Z

lgtm

you06

rest LGTM

you06 · 2024-08-08T03:15:01Z

internal/locate/region_cache_test.go

+	// region cache after insert: [[1, +inf)]
+
+	r, _ = s.cache.scanRegionsFromCache(s.bo, []byte{}, nil, 10)
+	s.Equal(len(r), 0) // fail in current implementation


Remove the comment // fail ...

you06 · 2024-08-08T03:25:25Z

internal/locate/region_cache_test.go

@@ -2259,13 +2260,35 @@ func (s *testRegionRequestToSingleStoreSuite) TestRefreshCache() {
 	s.cache.insertRegionToCache(&Region{meta: &r2, store: unsafe.Pointer(st), ttl: nextTTLWithoutJitter(time.Now().Unix())}, true, true)

 	r, _ = s.cache.scanRegionsFromCache(s.bo, []byte{}, nil, 10)
-	s.Equal(len(r), 2)
+	s.Equal(len(r), 1)


This is not related to this PR I think.

Since region cache doesn't remove the first intersected region(it scan intersected region by AscendGreaterOrEqual), the outdated region (-inf, inf) is still alive and the new inserted valid region [{1}, inf) is ignored because the first seen region (-inf, inf) contains all the required ranges... We finally got outdated region though we know some valid regions.

But this won't affect finding region by single key(it use descend scan).

I don't know if removing all intersected regions is a good idea, what about adding some comments explain it in this test?

Signed-off-by: wshwsh12 <[email protected]>

ti-chi-bot bot added the dco-signoff: yes Indicates the PR's author has signed the dco. label Jul 31, 2024

wshwsh12 force-pushed the region branch from 22857cc to 8eb7422 Compare July 31, 2024 09:15

you06 reviewed Aug 1, 2024

View reviewed changes

internal/locate/region_cache.go Show resolved Hide resolved

wshwsh12 requested a review from you06 August 1, 2024 07:46

you06 reviewed Aug 2, 2024

View reviewed changes

windtalker reviewed Aug 6, 2024

View reviewed changes

wshwsh12 added 2 commits August 6, 2024 14:36

try

ba810ec

add benchmark Signed-off-by: wshwsh12 <[email protected]>

impl

6d948eb

Signed-off-by: wshwsh12 <[email protected]>

wshwsh12 force-pushed the region branch from 6875c04 to 6d948eb Compare August 6, 2024 06:43

wshwsh12 added 2 commits August 6, 2024 16:20

time

32f5ff8

Signed-off-by: wshwsh12 <[email protected]>

fix ut

b1be139

Signed-off-by: wshwsh12 <[email protected]>

windtalker reviewed Aug 6, 2024

View reviewed changes

address comments

9e5fff0

Signed-off-by: wshwsh12 <[email protected]>

wshwsh12 requested review from you06 and windtalker August 6, 2024 09:01

windtalker reviewed Aug 6, 2024

View reviewed changes

startkey

649c6b2

Signed-off-by: wshwsh12 <[email protected]>

wshwsh12 requested a review from windtalker August 7, 2024 07:42

you06 reviewed Aug 8, 2024

View reviewed changes

add comments

81437b1

Signed-off-by: wshwsh12 <[email protected]>

wshwsh12 requested a review from you06 August 8, 2024 06:44

you06 approved these changes Aug 8, 2024

View reviewed changes

Merge branch 'master' into region

d9f132e

you06 merged commit f0f57f2 into tikv:master Aug 9, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

region cache: batch find regions by key ranges from cache #1410

region cache: batch find regions by key ranges from cache #1410

wshwsh12 commented Jul 31, 2024

you06 Aug 2, 2024

you06 Aug 2, 2024

wshwsh12 Aug 2, 2024 •

edited

Loading

you06 Aug 2, 2024

you06 Aug 2, 2024

wshwsh12 Aug 5, 2024

windtalker Aug 6, 2024

windtalker Aug 6, 2024

wshwsh12 Aug 6, 2024

windtalker Aug 6, 2024

windtalker Aug 6, 2024

windtalker Aug 6, 2024

windtalker Aug 6, 2024

windtalker Aug 6, 2024

wshwsh12 Aug 7, 2024

you06 Aug 7, 2024

wshwsh12 Aug 7, 2024

windtalker commented Aug 7, 2024

you06 left a comment

you06 Aug 8, 2024

you06 Aug 8, 2024 •

edited

Loading

region cache: batch find regions by key ranges from cache #1410

region cache: batch find regions by key ranges from cache #1410

Conversation

wshwsh12 commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wshwsh12 Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windtalker commented Aug 7, 2024

you06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

you06 Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

wshwsh12 Aug 2, 2024 •

edited

Loading

you06 Aug 8, 2024 •

edited

Loading