Skip to content

Comments

Performance Optimizations targeting to reduce CPU churn#73

Merged
prateek merged 13 commits intouber-java:masterfrom
alexeykudinkin:mp-ff-perf-opt
Jul 27, 2020
Merged

Performance Optimizations targeting to reduce CPU churn#73
prateek merged 13 commits intouber-java:masterfrom
alexeykudinkin:mp-ff-perf-opt

Conversation

@alexeykudinkin
Copy link
Contributor

Rationale for the change could be found in T6248475.

In a nutshell:

  1. Avoiding string format in the hotpath
  2. Replacing CHM traversal with single queue traversal (this will be leveraged in the follow-up)

@CLAassistant
Copy link

CLAassistant commented Jul 16, 2020

CLA assistant check
All committers have signed the CLA.

@alexeykudinkin
Copy link
Contributor Author

Currently failing test M3ReporterTest is fixed in #71

Copy link
Collaborator

@prateek prateek left a comment

Choose a reason for hiding this comment

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

few questions, mainly looking fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

why comment this out? i'd prefer if you either add in or remove entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is cleaned up in #74

Copy link
Collaborator

Choose a reason for hiding this comment

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

am I missing something or is this an unused import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaning up in #74

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't live in Java code so this more a curiosity: why override this method if you're going to only call the parent's method? Is it required by the Java type system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a refactoring aberration, cleaned up in #74

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove the TODO, it needs the concurrency because metrics can be both created concurrently, and created and reported upon concurrently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up in #74

m3/build.gradle Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

why remove this? does the m3 module inherit the jmh settings from the parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no benchmarks in this project

@alexeykudinkin alexeykudinkin requested a review from prateek July 23, 2020 00:26

Choose a reason for hiding this comment

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

Tally client already has heavy memory footprint, what's the memory impact of the additional queue?
Also how much cpu reduction does switching from traversing the hashmaps to traversing the queue? It is also not clear to me why that would reduce cpu usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reduce the traversal scope to only those metrics that had actually changed. This is leveraged in #74.

@prateek prateek merged commit 6d3c810 into uber-java:master Jul 27, 2020
sairamch04 pushed a commit to sairamch04/tally that referenced this pull request Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants