PCP: /api/v1/regions optimization #1970

ekalinin · 2019-11-25T21:32:27Z

What problem does this PR solve?

What is changed and how it works?

convertToAPIRegions optimization

reduced the amount of allocations in convertToAPIRegions by making a slice of RegionInfo.
added a new function - InitRegion. Used it in a loop to init a slice
of pointers to the api.RegionInfo structs without additional allocations.
added a method HexRegionKeyStr. It returns a string (and not a []byte).
StartKey/EndKey fields are string in the api.RegionInfo, and we do not need one more convertation (and allocation) from []byte to string.
added benchmarks:
- BenchmarkConvertToAPIRegions
- BenchmarkHexRegionKey
- BenchmarkHexRegionKeyStr

Result:

# GO111MODULE=on go test github.com/pingcap/pd/server/api -run=None -bench=BenchmarkConvertToAPIRegions -benchmem > new.api.txt
➜ benchcmp old.api.txt new.api.txt
benchmark                           old ns/op     new ns/op     delta
BenchmarkConvertToAPIRegions-12     2880085       2208890       -23.30%

benchmark                           old allocs    new allocs    delta
BenchmarkConvertToAPIRegions-12     82182         52183         -36.50%

benchmark                           old bytes     new bytes     delta
BenchmarkConvertToAPIRegions-12     2162007       1947634       -9.92%

h.rd.JSON optimization

added server/api/router.go#createRender function for creating *render.Render
removed option IndentJSON from the render.New call
added option StreamingJSON to the render.New call
added server/api/region_test.go#BenchmarkRenderJSON for testing the performance of the last part of the regionsHandler.GetAll function (which takes 90% of the total time for the /pd/api/v1/regions endpoint in PCP PCP-24: Improve the performance of the HTTP API for getting all regions #1837)

# GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkRenderJSON -benchmem > new.txt
➜ benchcmp old.txt new.txt
benchmark                  old ns/op     new ns/op     delta
BenchmarkRenderJSON-12     662659672     147632381     -77.72%

benchmark                  old allocs    new allocs    delta
BenchmarkRenderJSON-12     50            12            -76.00%

benchmark                  old bytes     new bytes     delta
BenchmarkRenderJSON-12     414771916     51753928      -87.52%

Check List

Tests

Unit test
Integration test

sre-bot · 2019-11-25T21:32:30Z

Thanks for your contribution. If your PR get merged, you will be rewarded 900 points.

nolouch · 2019-11-26T03:26:28Z

Thanks, @ekalinin, Greatly work.
In the origin issue, the convertToAPIRegions also allocated much memory, can we do something for this?

ekalinin · 2019-11-26T09:15:32Z

In the origin issue, the convertToAPIRegions also allocated much memory, can we do something for this?

Sure, will take a look on it today.

- reduced the amount of allocation in convertToAPIRegions by making a slice of RegionInfo. - added a new function - InitRegion. Used it in a loop to init a slice of pointers to the RegionInfo structs without additional allocations. - added method HexRegionKeyStr. It returns string (and not []byte). StartKey/EndKey are string, and we do not need one more convert (and allocation) from []byte to string. - added benchmarks: - BenchmarkConvertToAPIRegions - BenchmarkHexRegionKey - BenchmarkHexRegionKeyStr Result: ➜ benchcmp old.api.txt new.api.txt benchmark old ns/op new ns/op delta BenchmarkConvertToAPIRegions-12 2880085 2208890 -23.30% benchmark old allocs new allocs delta BenchmarkConvertToAPIRegions-12 82182 52183 -36.50% benchmark old bytes new bytes delta BenchmarkConvertToAPIRegions-12 2162007 1947634 -9.92% ➜ GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkHexRegionKey -benchmem goos: linux goarch: amd64 pkg: github.com/pingcap/pd/server/api BenchmarkHexRegionKey-12 1229766 1239 ns/op 240 B/op 5 allocs/op BenchmarkHexRegionKeyStr-12 1226398 1018 ns/op 144 B/op 3 allocs/op

codecov-io · 2019-11-26T22:02:00Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a87c9eb). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1970   +/-   ##
========================================
  Coverage          ?   77.9%           
========================================
  Files             ?     174           
  Lines             ?   17626           
  Branches          ?       0           
========================================
  Hits              ?   13732           
  Misses            ?    2834           
  Partials          ?    1060

Impacted Files	Coverage Δ
server/core/region.go	`91.49% <100%> (ø)`
server/api/router.go	`99.19% <100%> (ø)`
server/api/region.go	`77.25% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a87c9eb...fd26ae2. Read the comment docs.

nolouch

Hi, thanks @ekalinin, you have done several significant optimizations, but may we can still try more for :

Conversion between []byte and string may have memory allocation, you have added a function HexRegionKeyStr to reduce one conversion, but there still have some conversions. maybe we can try this method to do something.
strings.Upper() will do a copy, we can improve it to zero-copy with doing it in slice[]byte.
hex.EncodingToString also have a conversion.

server/api/region_test.go

server/api/region.go

- tRegions: removed args, used predefined args - added Slice/String functions for zero-cost convertations - added ToUpperASCIIInplace function (zero cost ToUppper) - added EncodeToString. It overrides hex.EncodeToString. Difference: returns []byte, not string

ekalinin · 2019-11-27T11:35:48Z

Changes:

region_test.newTestRegions: removed args, used predefined args
added String function for zero-cost conversation from []byte to string
added ToUpperASCIIInplace function (zero cost copy of ToUppper)
added EncodeToString. It overrides hex.EncodeToString. Difference: returns []byte, not string

Results:

# GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkHexRegionKey -benchmem

➜ benchcmp hex.old.txt hex.new.txt 
benchmark                       old ns/op     new ns/op     delta
BenchmarkHexRegionKey-12        532           137           -74.25%
BenchmarkHexRegionKeyStr-12     470           140           -70.21%

benchmark                       old allocs     new allocs     delta
BenchmarkHexRegionKey-12        5              1              -80.00%
BenchmarkHexRegionKeyStr-12     3              1              -66.67%

benchmark                       old bytes     new bytes     delta
BenchmarkHexRegionKey-12        400           48            -88.00%
BenchmarkHexRegionKeyStr-12     240           48            -80.00%

and in general:

➜ benchcmp old.api.txt new.api.txt
benchmark                           old ns/op     new ns/op     delta
BenchmarkConvertToAPIRegions-12     2880085       1550980       -46.15%

benchmark                           old allocs     new allocs     delta
BenchmarkConvertToAPIRegions-12     82182          20003          -75.66%

benchmark                           old bytes     new bytes     delta
BenchmarkConvertToAPIRegions-12     2162007       2002431       -7.38%

…CP-24/json-streaming

ekalinin · 2019-11-27T11:58:43Z

/run-all-tests

nolouch

I do the test same as the original issue, with 1,000,000 regions:
before

[2019/10/23 12:19:55.653 +00:00] [INFO] [region.go:154] ["[DEBUG] got all regions from core"] [cost=149.863003ms]
[2019/10/23 12:19:59.083 +00:00] [INFO] [region.go:157] ["[DEBUG] convert all regions to API regions"] [cost=3.430113877s]
[2019/10/23 12:20:17.575 +00:00] [INFO] [region.go:160] ["[DEBUG] send all regions to client"] [cost=18.491626948s]

after

[2019/11/27 15:15:14.985 +00:00] [INFO] [region.go:155] ["[DEBUG] got all regions from core"] [cost=31.286284ms]
[2019/11/27 15:15:16.834 +00:00] [INFO] [region.go:158] ["[DEBUG] convert all regions to API regions"] [cost=1.849013297s]
[2019/11/27 15:15:22.415 +00:00] [INFO] [region.go:161] ["[DEBUG] send all regions to client"] [cost=5.580648785s]

It's almost 3.5x faster now. but 5s may still too long, can we have more method to optimize it?
and I have got the profile from my test:
cpu.new.pprof.zip
mem.new.pprof.zip

maybe we need to focus on reducing CPU usage now:

how about have a try https://github.com/json-iterator/go to getting all regions?*
gzip the response may speed up transmission*

What is your opinion？

server/api/router.go

ekalinin · 2019-11-28T13:28:49Z

I do the test same as the original issue, with 1,000,000 regions:

Could you add a benchmark which would be close to that test with 1M regions?

It's almost 3.5x faster now.

Thanks! That's great!

but 5s may still too long, can we have more method to optimize it?
maybe we need to focus on reducing CPU usage now:

Sure, will try to dig into that direction.

We really need IndentJSON for human readable. can we keep them both at the same time?

Unfortunately, IndentJSON requires a lot of additional work (Marshal, then Indent):

https://github.com/golang/go/blob/8a5af7910a9b157c02736c3e0998a587bb8511c1/src/encoding/json/encode.go#L175-L186

Why just not use external tool? Like:

$ >> ./bin/pd-ctl -u http://0.0.0.0:32833 config show all | python -m json.tool

or

$ >> ./bin/pd-ctl -u http://0.0.0.0:32833 config show all | jq .

nolouch · 2019-11-28T15:48:43Z

Could you add a benchmark which would be close to that test with 1M regions?

I have shared my data in here, you can use it to construct the regions in your local benchmark.

Why just not use an external tool? Like

pd-ctl is a tool that our users or DBAs often use it to diagnose problems. , we must ensure that it is easy to use. at lease, we just let fetch all regions to use StreamingJSON, others don't.

ekalinin · 2019-11-30T20:47:22Z

at lease, we just let fetch all regions to use StreamingJSON, others don't.

Done.

ekalinin · 2019-11-30T23:03:41Z

how about have a try https://github.com/json-iterator/go to getting all regions?*

Just tried it. Looks not well:

# current branch

➜ GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkRenderJSON
goos: linux
goarch: amd64
pkg: github.com/pingcap/pd/server/api
BenchmarkRenderJSON-12    	      96	  15344540 ns/op	 2429223 B/op	       5 allocs/op
PASS
ok  	github.com/pingcap/pd/server/api	1.732s

# current branch with github.com/json-iterator/go

GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkRenderJSON
goos: linux
goarch: amd64
pkg: github.com/pingcap/pd/server/api
BenchmarkRenderJSON-12    	      27	  38831651 ns/op	 6267826 B/op	      18 allocs/op
PASS
ok  	github.com/pingcap/pd/server/api	1.328s

patch:

func (ri *RegionsInfo) MarshalJSON() ([]byte, error) {
	var json = jsoniter.ConfigFastest
	return json.Marshal(*ri)
}

ekalinin · 2019-11-30T23:36:21Z

Tried github.com/bet365/jingo as well. No luck again:

➜ GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkRenderJSON
goos: linux
goarch: amd64
pkg: github.com/pingcap/pd/server/api
BenchmarkRenderJSON-12    	      15	  77177199 ns/op	17316683 B/op	   50040 allocs/op
PASS
ok  	github.com/pingcap/pd/server/api	1.426s

patch:

var enc = jingo.NewStructEncoder(RegionInfo{})

func (ri *RegionInfo) MarshalJSON() ([]byte, error) {
 	buf := jingo.NewBufferFromPool()
 	enc.Marshal(ri, buf)

 	b := make([]byte, len(buf.Bytes))
 	copy(b, buf.Bytes)
 	buf.ReturnToPool()
 	return b, nil
}

nolouch

rest lgtm. Thank you for several optimization attempts.

server/api/router.go

ekalinin · 2019-12-01T15:28:40Z

Current results of the json render optimization:

# GO111MODULE=on go test github.com/pingcap/pd/server/api -run=Benchmark -bench=BenchmarkRenderJSON -benchmem

➜ benchcmp old.txt new.txt        
benchmark                  old ns/op     new ns/op     delta
BenchmarkRenderJSON-12     662659672     17829821      -97.31%

benchmark                  old allocs    new allocs    delta
BenchmarkRenderJSON-12     50            4             -92.00%

benchmark                  old bytes     new bytes     delta
BenchmarkRenderJSON-12     414771916     2339140       -99.44%

nolouch · 2019-12-02T03:16:33Z

PTAL @rleungx

lhy1024

LGTM.what format when we get all regions? would you like to provide examples?

rleungx

LGTM

sre-bot · 2019-12-02T07:55:57Z

/run-all-tests

sre-bot · 2019-12-02T08:00:04Z

@ekalinin complete task #1837 and get 900 score, currerent score 1450

Signed-off-by: Ryan Leung <rleungx@gmail.com>

… (#1986) * /api/v1/regions optimization (#1970) Signed-off-by: Ryan Leung <rleungx@gmail.com>

Signed-off-by: Ryan Leung <rleungx@gmail.com>

… (#1987) * /api/v1/regions optimization (#1970) * fix travis CI memory problem Signed-off-by: Ryan Leung <rleungx@gmail.com>

server: json api, drop indent, add streaming

b7ca970

Merge branch 'master' into PCP-24/json-streaming

549550c

nolouch added the contributor label Nov 26, 2019

Merge branch 'master' into PCP-24/json-streaming

8024668

ekalinin changed the title ~~PCP: json api, drop indent, add streaming~~ PCP: /api/v1/regions optimization Nov 26, 2019

server/api: updated tests for < go1.13

d251a84

nolouch requested review from shafreeck and disksing November 27, 2019 04:22

nolouch reviewed Nov 27, 2019

View reviewed changes

server/api/region_test.go Outdated Show resolved Hide resolved

server/api/region_test.go Outdated Show resolved Hide resolved

server/api/region_test.go Outdated Show resolved Hide resolved

rleungx reviewed Nov 27, 2019

View reviewed changes

server/api/region.go Outdated Show resolved Hide resolved

ekalinin and others added 2 commits November 27, 2019 14:25

Merge branch 'master' into PCP-24/json-streaming

bd8b41d

ekalinin added 3 commits November 27, 2019 14:36

server/api: fix test data

87bc407

Merge branch 'PCP-24/json-streaming' of github.com:ekalinin/pd into P…

7f3860e

…CP-24/json-streaming

server/api: remove Slice; re-ordered functions

c0b5f10

Merge branch 'master' into PCP-24/json-streaming

fc9ff60

nolouch reviewed Nov 27, 2019

View reviewed changes

server/api/router.go Show resolved Hide resolved

Merge branch 'master' into PCP-24/json-streaming

8da9a7a

server/api: streaming only for /api/v1/regions/*

4e5754f

nolouch reviewed Dec 1, 2019

View reviewed changes

server/api/router.go Outdated Show resolved Hide resolved

server/api: streaming render only for /api/v1/regions

fd26ae2

nolouch approved these changes Dec 2, 2019

View reviewed changes

lhy1024 reviewed Dec 2, 2019

View reviewed changes

rleungx approved these changes Dec 2, 2019

View reviewed changes

nolouch added the status/can-merge Indicates a PR has been approved by a committer. label Dec 2, 2019

sre-bot merged commit 9923a25 into tikv:master Dec 2, 2019

ekalinin deleted the PCP-24/json-streaming branch December 2, 2019 08:02

rleungx pushed a commit to rleungx/pd that referenced this pull request Dec 5, 2019

PCP: /api/v1/regions optimization (tikv#1970)

c6faa8b

rleungx mentioned this pull request Dec 5, 2019

Improve the performance of the HTTP API for getting all regions (#1970) #1986

Merged

rleungx pushed a commit to rleungx/pd that referenced this pull request Dec 5, 2019

/api/v1/regions optimization (tikv#1970)

c8cb4b4

rleungx pushed a commit to rleungx/pd that referenced this pull request Dec 5, 2019

/api/v1/regions optimization (tikv#1970)

bc2d3fa

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx pushed a commit to rleungx/pd that referenced this pull request Dec 5, 2019

/api/v1/regions optimization (tikv#1970)

6c96610

Signed-off-by: Ryan Leung <rleungx@gmail.com>

This was referenced Dec 5, 2019

Improve the performance of the HTTP API for getting all regions (#1970) #1987

Merged

Improve the performance of the HTTP API for getting all regions (#1970) #1988

Merged

nolouch pushed a commit that referenced this pull request Dec 5, 2019

Improve the performance of the HTTP API for getting all regions (#1970)…

e88372a

… (#1986) * /api/v1/regions optimization (#1970) Signed-off-by: Ryan Leung <rleungx@gmail.com>

nolouch pushed a commit that referenced this pull request Dec 5, 2019

/api/v1/regions optimization (#1970) (#1988)

79812a4

Signed-off-by: Ryan Leung <rleungx@gmail.com>

nolouch pushed a commit that referenced this pull request Dec 6, 2019

Improve the performance of the HTTP API for getting all regions (#1970)…

a25811a

… (#1987) * /api/v1/regions optimization (#1970) * fix travis CI memory problem Signed-off-by: Ryan Leung <rleungx@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PCP: /api/v1/regions optimization #1970

PCP: /api/v1/regions optimization #1970

ekalinin commented Nov 25, 2019 •

edited

Loading

sre-bot commented Nov 25, 2019

nolouch commented Nov 26, 2019

ekalinin commented Nov 26, 2019

codecov-io commented Nov 26, 2019 •

edited

Loading

nolouch left a comment •

edited

Loading

ekalinin commented Nov 27, 2019 •

edited

Loading

ekalinin commented Nov 27, 2019

nolouch left a comment •

edited

Loading

ekalinin commented Nov 28, 2019

nolouch commented Nov 28, 2019

ekalinin commented Nov 30, 2019

ekalinin commented Nov 30, 2019

ekalinin commented Nov 30, 2019

nolouch left a comment •

edited

Loading

ekalinin commented Dec 1, 2019

nolouch commented Dec 2, 2019

lhy1024 left a comment

rleungx left a comment

sre-bot commented Dec 2, 2019

sre-bot commented Dec 2, 2019

PCP: /api/v1/regions optimization #1970

PCP: /api/v1/regions optimization #1970

Conversation

ekalinin commented Nov 25, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Nov 25, 2019

nolouch commented Nov 26, 2019

ekalinin commented Nov 26, 2019

codecov-io commented Nov 26, 2019 • edited Loading

Codecov Report

nolouch left a comment • edited Loading

Choose a reason for hiding this comment

ekalinin commented Nov 27, 2019 • edited Loading

ekalinin commented Nov 27, 2019

nolouch left a comment • edited Loading

Choose a reason for hiding this comment

ekalinin commented Nov 28, 2019

nolouch commented Nov 28, 2019

ekalinin commented Nov 30, 2019

ekalinin commented Nov 30, 2019

ekalinin commented Nov 30, 2019

nolouch left a comment • edited Loading

Choose a reason for hiding this comment

ekalinin commented Dec 1, 2019

nolouch commented Dec 2, 2019

lhy1024 left a comment

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

sre-bot commented Dec 2, 2019

sre-bot commented Dec 2, 2019

ekalinin commented Nov 25, 2019 •

edited

Loading

codecov-io commented Nov 26, 2019 •

edited

Loading

nolouch left a comment •

edited

Loading

ekalinin commented Nov 27, 2019 •

edited

Loading

nolouch left a comment •

edited

Loading

nolouch left a comment •

edited

Loading