*: optimize the layout of global sort files and other #48275

wjhuang2016 · 2023-11-03T08:02:04Z

What problem does this PR solve?

Issue Number: ref #48779

Problem Summary:

What is changed and how it works?

The old layout is keyLen: uint64, key: char[keyLen], valueLen: uint64, value: char[valueLen]. In this PR we change it to keyLen: uint64, valueLen: uint64, key: char[keyLen], value: char[valueLen]. Compared with the old PR, we can merge reading key and value into one IO and split the data afterwards, and each IO does not require the previous data buffer to be retained in memory.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

benchmark of merge iter reading

$ go test ./br/pkg/lightning/backend/external -v --tags=intest -test.run TestCompareReader --testing-storage-uri "s3://xxx"

this PR:
    bench_test.go:414: sequential read speed for 1258291200 bytes: 52.64 MB/s
    bench_test.go:421: concurrent read speed for 1258291200 bytes: 575.51 MB/s
    bench_test.go:428: merge iter read speed for 1258291200 bytes: 150.50 MB/s

master:
    bench_test.go:413: sequential read speed for 1258291200 bytes: 51.65 MB/s
    bench_test.go:420: concurrent read speed for 1258291200 bytes: 516.10 MB/s
    bench_test.go:427: merge iter read speed for 1258291200 bytes: 112.53 MB/s

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

codecov · 2023-11-03T08:18:38Z

Codecov Report

Merging #48275 (56337f1) into master (3543275) will decrease coverage by 16.9222%.
Report is 13 commits behind head on master.
The diff coverage is 85.3658%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #48275         +/-   ##
=================================================
- Coverage   71.0083%   54.0861%   -16.9222%     
=================================================
  Files          1367       1582        +215     
  Lines        404865     596180     +191315     
=================================================
+ Hits         287488     322451      +34963     
- Misses        97372     251125     +153753     
- Partials      20005      22604       +2599

Flag	Coverage Δ
integration	`35.0588% <68.2926%> (?)`
unit	`71.3596% <85.3658%> (+0.3513%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`55.3775% <85.3658%> (+2.3006%)`	⬆️

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

Signed-off-by: lance6716 <lance6716@gmail.com>

br/pkg/lightning/backend/external/bench_test.go

tangenta · 2023-11-23T06:07:05Z

br/pkg/lightning/backend/external/byte_reader.go

 	}
 	// If the reader has fewer than n bytes remaining in current buffer,
 	// `auxBuf` is used as a container instead.
+	if n > 1024*1024*1024 {


Why is it 1024*1024*1024?

I think it's a defensive coding, and the author of this line is @wjhuang2016 let's ask him

Yes，if there is something wrong we can return an error instead of panic. Just choose a large number.

I think it is better to do this outside of readNBytes() and log the file name & other info.

Could you change this to 1 * size.GB instead and add comment for it?

Signed-off-by: lance6716 <lance6716@gmail.com>

br/pkg/lightning/backend/external/writer.go

Signed-off-by: lance6716 <lance6716@gmail.com>

ywqzzy · 2023-11-24T02:17:55Z

br/pkg/lightning/backend/external/writer.go

+	binary.BigEndian.AppendUint64(dataBuf[:lengthBytes], uint64(len(idxVal)))
+	keyAdapter.Encode(dataBuf[2*lengthBytes:2*lengthBytes:2*lengthBytes+encodedKeyLen], idxKey, rowID)
+	copy(dataBuf[2*lengthBytes+encodedKeyLen:], idxVal)


onefile writer should change too

I have changed the order to keyLen, valueLen, key, value in onefile_writer.go line 108~111.

br/pkg/lightning/backend/external/writer.go

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-11-24T05:41:33Z

ptal @ywqzzy @tangenta

ti-chi-bot · 2023-11-24T08:03:46Z

[LGTM Timeline notifier]

Timeline:

2023-11-24 08:02:57.195123157 +0000 UTC m=+564205.860349352: ☑️ agreed by tangenta.
2023-11-24 08:03:45.364566666 +0000 UTC m=+564254.029792862: ☑️ agreed by lance6716.

ti-chi-bot · 2023-11-24T08:06:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, tangenta, YuJuncen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~br/OWNERS~~ [YuJuncen]
~~br/pkg/lightning/OWNERS~~ [lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lance6716 · 2023-11-24T08:07:45Z

/retest

lance6716 · 2023-11-24T08:34:11Z

/retest

lance6716 · 2023-11-24T10:33:39Z

/retest

try

a2e73b8

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

ti-chi-bot bot added do-not-merge/invalid-title do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 3, 2023

try

964adab

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 3, 2023

wjhuang2016 added 2 commits November 3, 2023 16:56

fix

09c267e

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

0ffe2e0

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

wjhuang2016 force-pushed the test_master branch from 0ed326c to 0ffe2e0 Compare November 6, 2023 10:12

wjhuang2016 added 4 commits November 6, 2023 18:38

try

c4074b3

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

280c20f

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

fffd17c

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

74a817c

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2023

wjhuang2016 added 10 commits November 6, 2023 20:06

fix

19c4e1e

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

619385d

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

3450098

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

fix

1bdde22

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

11752f2

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

fix

6649072

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

262d459

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

454a032

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

try

8f4cfd4

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

refine

260022a

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

ti-chi-bot bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 7, 2023

fix comment

d61393d

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 changed the title *: optimize the layout of global sort files *: optimize the layout of global sort files and other Nov 22, 2023

remove membuf

cccdeeb

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Nov 22, 2023

fix CI

8ce124e

Signed-off-by: lance6716 <lance6716@gmail.com>

Benjamin2037 reviewed Nov 22, 2023

View reviewed changes

br/pkg/lightning/backend/external/bench_test.go Outdated Show resolved Hide resolved

revert some change

5375d41

tangenta reviewed Nov 23, 2023

View reviewed changes

lance6716 added 2 commits November 23, 2023 18:17

Merge branch 'master' of github.com:pingcap/tidb into test_master

56063a3

Signed-off-by: lance6716 <lance6716@gmail.com>

fix build

00d28ad

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta reviewed Nov 23, 2023

View reviewed changes

br/pkg/lightning/backend/external/writer.go Show resolved Hide resolved

tangenta reviewed Nov 23, 2023

View reviewed changes

br/pkg/lightning/backend/external/writer.go Outdated Show resolved Hide resolved

address comment

6e59d90

Signed-off-by: lance6716 <lance6716@gmail.com>

ywqzzy reviewed Nov 24, 2023

View reviewed changes

tangenta reviewed Nov 24, 2023

View reviewed changes

br/pkg/lightning/backend/external/writer.go Outdated Show resolved Hide resolved

address comment

56337f1

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta approved these changes Nov 24, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 24, 2023

lance6716 approved these changes Nov 24, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 24, 2023

YuJuncen approved these changes Nov 24, 2023

View reviewed changes

ti-chi-bot bot added the approved label Nov 24, 2023

ti-chi-bot bot merged commit 1263445 into pingcap:master Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: optimize the layout of global sort files and other #48275

*: optimize the layout of global sort files and other #48275

wjhuang2016 commented Nov 3, 2023 •

edited by lance6716

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

tangenta Nov 23, 2023

lance6716 Nov 23, 2023

wjhuang2016 Nov 23, 2023

tangenta Nov 23, 2023

ywqzzy Nov 24, 2023

lance6716 Nov 24, 2023

lance6716 commented Nov 24, 2023

ti-chi-bot bot commented Nov 24, 2023

ti-chi-bot bot commented Nov 24, 2023

lance6716 commented Nov 24, 2023

lance6716 commented Nov 24, 2023

lance6716 commented Nov 24, 2023

*: optimize the layout of global sort files and other #48275

*: optimize the layout of global sort files and other #48275

Conversation

wjhuang2016 commented Nov 3, 2023 • edited by lance6716 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

benchmark of merge iter reading

Release note

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

tangenta Nov 23, 2023

Choose a reason for hiding this comment

lance6716 Nov 23, 2023

Choose a reason for hiding this comment

wjhuang2016 Nov 23, 2023

Choose a reason for hiding this comment

tangenta Nov 23, 2023

Choose a reason for hiding this comment

ywqzzy Nov 24, 2023

Choose a reason for hiding this comment

lance6716 Nov 24, 2023

Choose a reason for hiding this comment

lance6716 commented Nov 24, 2023

ti-chi-bot bot commented Nov 24, 2023

[LGTM Timeline notifier]

ti-chi-bot bot commented Nov 24, 2023

lance6716 commented Nov 24, 2023

lance6716 commented Nov 24, 2023

lance6716 commented Nov 24, 2023

wjhuang2016 commented Nov 3, 2023 •

edited by lance6716

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading