optimization: using mashal to replace proto clone #5480

bufferflies · 2022-09-08T06:38:58Z

Signed-off-by: bufferflies [email protected]

What problem does this PR solve?

Issue Number: Close #5477

What is changed and how does it work?

Check List

Tests

Unit test
Integration test

benchmark

Code changes

Side effects

Related changes

Release note

 None.

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

ti-chi-bot · 2022-09-08T06:38:59Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

bufferflies · 2022-09-08T06:41:21Z

some discuss about generics create link: golang/go#52151

rleungx · 2022-09-08T06:57:11Z

Would you like to add a benchmark to compare these two methods?

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

codecov · 2022-09-08T07:42:17Z

Codecov Report

Base: 75.58% // Head: 75.64% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (5dd99ae) compared to base (d269010).
Patch coverage: 92.59% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5480      +/-   ##
==========================================
+ Coverage   75.58%   75.64%   +0.06%     
==========================================
  Files         323      325       +2     
  Lines       32000    32017      +17     
==========================================
+ Hits        24186    24220      +34     
+ Misses       5726     5708      -18     
- Partials     2088     2089       +1

Flag	Coverage Δ
unittests	`75.64% <92.59%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/typeutil/clone.go	`70.00% <70.00%> (ø)`
pkg/mock/mockcluster/mockcluster.go	`94.17% <75.00%> (ø)`
pkg/encryption/region_crypter.go	`67.92% <100.00%> (ø)`
server/cluster/cluster.go	`82.64% <100.00%> (-0.54%)`	⬇️
server/cluster/cluster_worker.go	`70.00% <100.00%> (ø)`
server/core/factory.go	`100.00% <100.00%> (ø)`
server/core/region.go	`91.28% <100.00%> (ø)`
server/core/store.go	`82.18% <100.00%> (-0.26%)`	⬇️
server/core/store_option.go	`100.00% <100.00%> (ø)`
server/schedule/filter/filters.go	`91.81% <100.00%> (ø)`
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

bufferflies · 2022-09-09T08:41:26Z

DeepClone Flame

ProtoClone Flame

pkg/typeutil/clone.go

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

pkg/typeutil/clone_test.go

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

pkg/typeutil/clone.go

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

pkg/typeutil/clone.go

server/core/region.go

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

rleungx

The rest LGTM.

server/core/store.go

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

server/core/store_option.go

server/core/store_test.go

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

bufferflies · 2022-09-18T07:33:52Z

PTAL @nolouch again

nolouch · 2022-09-19T07:26:30Z

/merge

ti-chi-bot · 2022-09-19T07:26:31Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-09-19T07:26:32Z

This pull request has been accepted and is ready to merge.

Commit hash: 5dd99ae

use marshal to replace proto.clone

05080e1

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

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 8, 2022

ti-chi-bot requested review from JmPotato and rleungx September 8, 2022 06:39

return nil if src is nil

edf270d

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

bufferflies force-pushed the proto-clone branch from 06a0b7d to edf270d Compare September 8, 2022 07:32

add benchmark

4dab0d8

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

nolouch reviewed Sep 13, 2022

View reviewed changes

pkg/typeutil/clone.go Outdated Show resolved Hide resolved

remove reflect

eba2422

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

bufferflies force-pushed the proto-clone branch from e1a00b3 to eba2422 Compare September 14, 2022 04:10

rleungx reviewed Sep 14, 2022

View reviewed changes

pkg/typeutil/clone_test.go Outdated Show resolved Hide resolved

rleungx reviewed Sep 14, 2022

View reviewed changes

pkg/typeutil/clone_test.go Outdated Show resolved Hide resolved

bufferflies added 2 commits September 14, 2022 16:02

refactor

41f3626

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

address comment

4ed833f

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

bufferflies force-pushed the proto-clone branch from ff97bd2 to 4ed833f Compare September 14, 2022 10:20

bufferflies added 2 commits September 14, 2022 18:58

Merge branch 'master' into proto-clone

8628426

comment

a276431

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

nolouch reviewed Sep 15, 2022

View reviewed changes

pkg/typeutil/clone.go Show resolved Hide resolved

bufferflies added 2 commits September 15, 2022 11:17

comment

b3d62be

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

address comment

d4edb3d

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

rleungx reviewed Sep 15, 2022

View reviewed changes

pkg/typeutil/clone.go Outdated Show resolved Hide resolved

bufferflies force-pushed the proto-clone branch 3 times, most recently from a04936a to da094d9 Compare September 15, 2022 09:54

rleungx reviewed Sep 15, 2022

View reviewed changes

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

bufferflies force-pushed the proto-clone branch 2 times, most recently from 4999a36 to 9db407a Compare September 15, 2022 10:04

move to core

5574f4f

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

bufferflies force-pushed the proto-clone branch from 9db407a to 5574f4f Compare September 15, 2022 11:06

Merge branch 'master' into proto-clone

13e7646

bufferflies requested review from nolouch and rleungx September 16, 2022 01:12

rleungx approved these changes Sep 16, 2022

View reviewed changes

server/core/store.go Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 16, 2022

bufferflies added 2 commits September 16, 2022 12:57

remove cloneMetaStore

1f1a392

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

Merge branch 'proto-clone' of github.com:bufferflies/pd into proto-clone

e5e457f

rleungx reviewed Sep 16, 2022

View reviewed changes

server/core/store_option.go Outdated Show resolved Hide resolved

server/core/store_test.go Outdated Show resolved Hide resolved

import

b2c24a5

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

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2022

resolve conflict

5dd99ae

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

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2022

nolouch approved these changes Sep 19, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 19, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 19, 2022

ti-chi-bot merged commit 1e1732c into tikv:master Sep 19, 2022

bufferflies deleted the proto-clone branch September 20, 2022 02:21

okJiang mentioned this pull request Jun 17, 2024

Not need json.Unmarshal when Clone(), which is time-consuming #8298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimization: using mashal to replace proto clone #5480

optimization: using mashal to replace proto clone #5480

bufferflies commented Sep 8, 2022 •

edited

Loading

ti-chi-bot commented Sep 8, 2022 •

edited

Loading

bufferflies commented Sep 8, 2022

rleungx commented Sep 8, 2022 •

edited

Loading

codecov bot commented Sep 8, 2022 •

edited

Loading

bufferflies commented Sep 9, 2022

rleungx left a comment

bufferflies commented Sep 18, 2022

nolouch commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

optimization: using mashal to replace proto clone #5480

optimization: using mashal to replace proto clone #5480

Conversation

bufferflies commented Sep 8, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Sep 8, 2022 • edited Loading

bufferflies commented Sep 8, 2022

rleungx commented Sep 8, 2022 • edited Loading

codecov bot commented Sep 8, 2022 • edited Loading

Codecov Report

bufferflies commented Sep 9, 2022

rleungx left a comment

Choose a reason for hiding this comment

bufferflies commented Sep 18, 2022

nolouch commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

bufferflies commented Sep 8, 2022 •

edited

Loading

ti-chi-bot commented Sep 8, 2022 •

edited

Loading

rleungx commented Sep 8, 2022 •

edited

Loading

codecov bot commented Sep 8, 2022 •

edited

Loading