Skip to content

*: use strings.Builder instead of bytes.Buffer#15908

Merged
ahrtr merged 1 commit intoetcd-io:mainfrom
cuishuang:main
May 26, 2023
Merged

*: use strings.Builder instead of bytes.Buffer#15908
ahrtr merged 1 commit intoetcd-io:mainfrom
cuishuang:main

Conversation

@cuishuang
Copy link
Copy Markdown
Contributor

@cuishuang cuishuang commented May 16, 2023 •
edited
Loading

strings.Builder uses *(*string)(unsafe.Pointer(&b.buf)) to save the application memory operation when converting to a string

Updates #15910

lavacat
lavacat suggested changes May 16, 2023
View reviewed changes
Comment thread pkg/cobrautl/help.go
Comment thread server/etcdserver/api/rafthttp/snapshot_sender.go Outdated
@lavacat
Copy link
Copy Markdown

lavacat commented May 16, 2023

Thanks for submitting the PR. Please open an issue and link
https://go-review.googlesource.com/c/go/+/102479
https://go-review.googlesource.com/c/go/+/96980
As proof that this is needed.

@cuishuang
Copy link
Copy Markdown
Contributor Author

cuishuang commented May 16, 2023

Thanks for submitting the PR. Please open an issue and link https://go-review.googlesource.com/c/go/+/102479 https://go-review.googlesource.com/c/go/+/96980 As proof that this is needed.

Thanks for the suggestion. added

lavacat
lavacat suggested changes May 17, 2023
View reviewed changes
Copy link
Copy Markdown

@lavacat lavacat left a comment

Choose a reason for hiding this comment

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

please make sure code is compiling: make build

Comment thread server/etcdserver/api/membership/cluster.go
Comment thread server/etcdserver/api/rafthttp/snapshot_sender.go
@lavacat
Copy link
Copy Markdown

lavacat commented May 18, 2023

@cuishuang please address failing "Static Analysis" check.

the following files are not sync with 'goimports'. run 'make fix'
[430](https://github.com/etcd-io/etcd/actions/runs/5004332677/jobs/8966774215?pr=15908#step:6:431)
/home/runner/work/etcd/etcd/server/etcdserver/api/rafthttp/snapshot_sender.go
[431](https://github.com/etcd-io/etcd/actions/runs/5004332677/jobs/8966774215?pr=15908#step:6:432)
There was a Failure in module server, aborting...

@cuishuang
use the more efficient strings.Builder
0c919dc 
Signed-off-by: cui fliter <imcusg@gmail.com>
@cuishuang
Copy link
Copy Markdown
Contributor Author

cuishuang commented May 19, 2023

@cuishuang please address failing "Static Analysis" check.

the following files are not sync with 'goimports'. run 'make fix'
[430](https://github.com/etcd-io/etcd/actions/runs/5004332677/jobs/8966774215?pr=15908#step:6:431)
/home/runner/work/etcd/etcd/server/etcdserver/api/rafthttp/snapshot_sender.go
[431](https://github.com/etcd-io/etcd/actions/runs/5004332677/jobs/8966774215?pr=15908#step:6:432)
There was a Failure in module server, aborting...

solved, thanks

@lavacat
Copy link
Copy Markdown

lavacat commented May 21, 2023

nit: please change commit msg to *: use strings.Builder instead of bytes.Buffer

@lavacat
Copy link
Copy Markdown

lavacat commented May 21, 2023

I think this is ok to merge because it's a best practice. In terms of performance it might only matter in id.go and trace.go. This covers all occurrences except for tests. cc @serathius @ahrtr

ahrtr
ahrtr approved these changes May 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ahrtr ahrtr 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

@cuishuang cuishuang changed the title use the more efficient strings.Builder *: use strings.Builder instead of bytes.Buffer May 22, 2023
@cuishuang
Copy link
Copy Markdown
Contributor Author

cuishuang commented May 22, 2023

nit: please change commit msg to *: use strings.Builder instead of bytes.Buffer

Done. Thanks

@ahrtr ahrtr merged commit 3413f2e into etcd-io:main May 26, 2023
@cuishuang cuishuang mentioned this pull request Jun 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serathius serathius serathius approved these changes

@ahrtr ahrtr ahrtr approved these changes

+1 more reviewer

@lavacat lavacat lavacat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cuishuang @lavacat @serathius @ahrtr