Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Change Monitor stat history to sync.Map#3007

Merged
dneuman64 merged 3 commits intoapache:masterfrom
rob05c:tm-fix-unnecessary-structs-plus-stat-history-syncmap
Nov 19, 2018
Merged

Change Monitor stat history to sync.Map#3007
dneuman64 merged 3 commits intoapache:masterfrom
rob05c:tm-fix-unnecessary-structs-plus-stat-history-syncmap

Conversation

@rob05c
Copy link
Copy Markdown
Member

@rob05c rob05c commented Nov 9, 2018

What does this PR do?

Based on #3000 - recommend merging that first.

Change Monitor stat history to sync.Map

This significantly improves performance. In my testing, for a large
CDN, processing Stat results can take ~5 seconds, where ~1s of that
was copying and setting the mutexed stat history variable. This
essentially eliminates that 1s or 1/5 of stat processing time, for
every stat process. This significantly reduces CPU, and/or makes
the monitor able to poll stats in significantly less time.

Also changes the stat history array to not reallocate memory,
but move the data when prepending a new result. Which is also
significantly faster, in my testing.

Which TC components are affected by this PR?

  • Documentation
  • Grove
  • Traffic Analytics
  • Traffic Monitor
  • Traffic Ops
  • Traffic Ops ORT
  • Traffic Portal
  • Traffic Router
  • Traffic Stats
  • Traffic Vault
  • Other _________

What is the best way to verify this PR?

Run Monitor, verify health and stats are polled correctly. This PR does not change the interface; it's an internal performance improvement.

Check all that apply

  • This PR includes tests
  • This PR includes documentation updates
  • This PR includes an update to CHANGELOG.md
  • This PR includes all required license headers
  • This PR includes a database migration (ensure that migration sequence is correct)
  • This PR fixes a serious security flaw. Read more: www.apache.org/security

@rob05c rob05c added new feature A new feature, capability or behavior Traffic Monitor related to Traffic Monitor labels Nov 9, 2018
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 9, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2759/
Test PASSed.

Copy link
Copy Markdown
Contributor

@dneuman64 dneuman64 left a comment

Choose a reason for hiding this comment

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

I know @ezelkow1 took a look at the code changes here, but I think this PR would benefit from a test or two.

@rob05c rob05c force-pushed the tm-fix-unnecessary-structs-plus-stat-history-syncmap branch 2 times, most recently from f997480 to 480d6c2 Compare November 18, 2018 06:02
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 18, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2801/
Test FAILed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 18, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2803/
Test FAILed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 18, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2804/
Test FAILed.

@rob05c rob05c force-pushed the tm-fix-unnecessary-structs-plus-stat-history-syncmap branch from f2368c1 to f234499 Compare November 18, 2018 16:51
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 18, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2805/
Test FAILed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 19, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2806/
Test FAILed.

This significantly improves performance. In my testing, for a large
CDN, processing Stat results can take ~5 seconds, where ~1s of that
was copying and setting the mutexed stat history variable. This
essentially eliminates that 1s or 1/5 of stat processing time, for
every stat process. This significantly reduces CPU, and/or makes
the monitor able to poll stats in significantly less time.

Also changes the stat history array to not reallocate memory,
but move the data when prepending a new result. Which is also
significantly faster, in my testing.
As required by PR Review.
@rob05c rob05c force-pushed the tm-fix-unnecessary-structs-plus-stat-history-syncmap branch from d94b6db to 19b1255 Compare November 19, 2018 15:43
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 19, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2812/
Test FAILed.

@dneuman64 dneuman64 merged commit 49df3d4 into apache:master Nov 19, 2018
@rob05c rob05c deleted the tm-fix-unnecessary-structs-plus-stat-history-syncmap branch January 2, 2019 20:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new feature A new feature, capability or behavior Traffic Monitor related to Traffic Monitor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants