Skip to content
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

Optimize load rules for max size rule #176

Merged
merged 1 commit into from
Jul 22, 2020
Merged

Optimize load rules for max size rule #176

merged 1 commit into from
Jul 22, 2020

Conversation

louyuting
Copy link
Collaborator

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@louyuting louyuting requested a review from sczyh30 June 27, 2020 16:07
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2020 •
edited
Loading

Codecov Report

Merging #176 into master will increase coverage by 0.12%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   44.62%   44.75%   +0.12%     
==========================================
  Files          78       78              
  Lines        4329     4357      +28     
==========================================
+ Hits         1932     1950      +18     
- Misses       2184     2188       +4     
- Partials      213      219       +6
Impacted Files Coverage Δ
core/hotspot/rule_manager.go 58.98% <77.08%> (+0.80%) ⬆️
core/flow/rule_manager.go 55.46% <77.77%> (+0.18%) ⬆️
core/circuitbreaker/rule_manager.go 50.56% <80.00%> (-1.49%) ⬇️
core/system/rule_manager.go 94.23% <100.00%> (+0.35%) ⬆️
core/hotspot/rule.go 66.25% <0.00%> (+2.50%) ⬆️

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 15fa39a...6e41449. Read the comment docs.

@louyuting louyuting added area/performance Issues or PRs related to runtime performance kind/enhancement Category issues or PRs related to enhancement to-review PRs to review labels Jun 27, 2020
@louyuting
Copy link
Collaborator Author

@sczyh30
This PR optimize the granularity of locks when LoadRules.

  • Keep the lock granularity to a minimum
  • Reduce memory allocates during write lock holding(avoid to grow map or slice)

But there are still some issues to discuss：

  • If the size of rules exceed 10k, the time read lock will cost 10ms+, it also causes cpu and memory jitter.

Some of my thinking is to sharding and updating the rule list, but this will bring some issues for reuse statistics.

Do you have more suggestion?

cncal
cncal reviewed Jun 29, 2020
View reviewed changes
tests/maxsize_rule_list_benchmark_test.go
rs := make([]cb.Rule, 0, 1000)
intervals := []uint32{10000, 15000, 20000, 25000, 30000}
for i := 0; i < 1000; i++ {
retryTimeout := intervals[rand.Int()%5]
Copy link
Contributor

Choose a reason for hiding this comment

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

rand.Int() will generate the same pseudo-random sequence if you don't call rand.Seed() first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@louyuting louyuting force-pushed the optimize_load_rules_for_max_size_rule branch 2 times, most recently from 825db3e to 8e52aa3 Compare July 3, 2020 12:00
@louyuting louyuting linked an issue Jul 3, 2020 that may be closed by this pull request
Improve the logic of updating flow rules #41
Closed
sczyh30
sczyh30 reviewed Jul 16, 2020
View reviewed changes
core/hotspot/rule_manager.go Outdated
defer tcMux.Unlock()
defer func() {
tcMux.Unlock()
logger.Infof("Updating hotspot rule spends %d ns.", util.CurrentTimeNano()-start)
Copy link
Member

Choose a reason for hiding this comment

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

Make it "debug" level?

core/hotspot/rule_manager.go
defer func() {
tcMux.Unlock()
logger.Infof("Updating hotspot rule spends %d ns.", util.CurrentTimeNano()-start)
logRuleUpdate(m)
Copy link
Member

Choose a reason for hiding this comment

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

Here we may need to handle cases regarding panic outside (where the log is not required).

@louyuting louyuting force-pushed the optimize_load_rules_for_max_size_rule branch from 8e52aa3 to 6e41449 Compare July 17, 2020 12:04
sczyh30
sczyh30 approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit ed802d3 into alibaba:master Jul 22, 2020
@sczyh30
Copy link
Member

sczyh30 commented Jul 22, 2020

Thanks!

@sczyh30 sczyh30 removed the to-review PRs to review label Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cncal cncal cncal left review comments

@sczyh30 sczyh30 sczyh30 approved these changes

Assignees
No one assigned
Labels
area/performance Issues or PRs related to runtime performance kind/enhancement Category issues or PRs related to enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the logic of updating flow rules
4 participants
@louyuting @codecov-commenter @sczyh30 @cncal