Skip to content

Refine string builder usage in logging pkg #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 15, 2020
Merged

Refine string builder usage in logging pkg #288

merged 1 commit into from
Oct 15, 2020

Conversation

@luckyxiaoqiang
Copy link
Collaborator

@luckyxiaoqiang luckyxiaoqiang commented Oct 13, 2020 •
edited
Loading

Describe what this PR does / why we need it

Use string builder grow to reduce memory allocations in logging pkg.

Benchmark result:

goos: darwin
goarch: amd64
pkg: github.com/alibaba/sentinel-golang/logging
BenchmarkAssembleMsg/assemble_msg_using_string_builder_with_grow-4                742695              1681 ns/op             624 B/op          8 allocs/op
BenchmarkAssembleMsg/assemble_msg_using_string_builder_without_grow-4             678778              1901 ns/op             728 B/op         12 allocs/op

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@luckyxiaoqiang luckyxiaoqiang changed the title Refine string builder usage Refine string builder usage in logging pkg Oct 13, 2020
louyuting
louyuting reviewed Oct 14, 2020
View reviewed changes
logging/logging.go Outdated
var (
DefaultDirName = filepath.Join("logs", "csp")

defaultLogMsgBufferSize = 256
Copy link
Collaborator

@louyuting louyuting Oct 14, 2020

Choose a reason for hiding this comment

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

maybe make it const is better?

Copy link
Collaborator Author

@luckyxiaoqiang luckyxiaoqiang Oct 14, 2020

Choose a reason for hiding this comment

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

A const is not convenient for benchmark test.

Copy link
Collaborator

@louyuting louyuting Oct 14, 2020

Choose a reason for hiding this comment

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

From my perspective, Benchmark is to test performance and shouldn't impact the code.

Copy link
Collaborator Author

@luckyxiaoqiang luckyxiaoqiang Oct 14, 2020

Choose a reason for hiding this comment

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

OK, I'll refine it.

Copy link
Collaborator Author

@luckyxiaoqiang luckyxiaoqiang Oct 14, 2020

Choose a reason for hiding this comment

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

OK, I'll refine it.

@louyuting louyuting added the area/logging Issue related to logging of Sentinel label Oct 14, 2020
@louyuting louyuting added this to the 1.0.0 milestone Oct 14, 2020
@codecov-io
Copy link

codecov-io commented Oct 14, 2020 •
edited
Loading

Codecov Report

Merging #288 into master will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
+ Coverage   51.09%   51.24%   +0.15%     
==========================================
  Files          79       79              
  Lines        4026     4049      +23     
==========================================
+ Hits         2057     2075      +18     
- Misses       1679     1684       +5     
  Partials      290      290
Impacted Files Coverage Δ
logging/logging.go 64.22% <100.00%> (+0.29%) ⬆️
core/base/slot_chain.go 92.95% <0.00%> (-4.06%) ⬇️
api/api.go 19.44% <0.00%> (-0.85%) ⬇️
core/flow/rule.go 19.04% <0.00%> (ø)
core/isolation/slot.go 0.00% <0.00%> (ø)
core/circuitbreaker/slot.go 100.00% <0.00%> (ø)
core/flow/rule_manager.go 61.36% <0.00%> (+0.29%) ⬆️
core/circuitbreaker/rule_manager.go 71.83% <0.00%> (+0.32%) ⬆️
core/system/rule_manager.go 80.76% <0.00%> (+0.37%) ⬆️
core/hotspot/rule_manager.go 53.37% <0.00%> (+0.57%) ⬆️
... and 8 more

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 cb639b4...1813496. Read the comment docs.

louyuting
louyuting approved these changes Oct 15, 2020
View reviewed changes
Copy link
Collaborator

@louyuting louyuting left a comment

Choose a reason for hiding this comment

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

Thanks for contribution!

@louyuting louyuting merged commit 7e1fad2 into alibaba:master Oct 15, 2020
@luckyxiaoqiang luckyxiaoqiang deleted the refine_string_builder branch December 27, 2020 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@louyuting louyuting louyuting approved these changes

Assignees

No one assigned

Labels

area/logging Issue related to logging of Sentinel

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@luckyxiaoqiang @codecov-io @louyuting