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.

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

Reduce overhead of metrics recording #74

Closed
wants to merge 1 commit into from
Closed

Conversation

njhill
Copy link
Contributor

@njhill njhill commented Mar 18, 2019

This is an optimization to reduce the amount of objects allocated when the various metrics are recorded, and minimize processing done on the application threads.

The various events are now passed in "raw" form via dedicated objects to the sender thread (rather than first being serialized to a string on the app thread). All of the serialization work is now done on the sender thread, avoiding allocation of any new strings or byte arrays. A single StringBuilder is reused in conjunction with the existing ByteBuffer. The NumberFormat ThreadLocals are no longer needed since they will now only be used by the one thread.

Also included are a couple of thread-safety tweaks to the unit tests.

This is an optimization to reduce the amount of objects allocated when the various metrics are recorded, and minimize processing done on the application threads.

The various events are now passed in "raw" form via dedicated objects to the sender thread (rather than first being serialized to a string on the app thread). All of the serialization work is now done on the sender thread, avoiding allocation of any new strings or byte arrays. A single StringBuilder is reused in conjunction with the existing ByteBuffer. The NumberFormat ThreadLocals are no longer needed since they will now only be used by the one thread.

Also included are a couple of thread-safety tweaks to the unit tests.
@njhill
Copy link
Contributor Author

njhill commented Mar 26, 2019

Now re-based on latest master branch. Any interest in this change?

@darxriggs
Copy link

@njhill there are merge conflicts that should be resolved.
(I am not the maintainer.)

@njhill
Copy link
Contributor Author

njhill commented Oct 15, 2019

@darxriggs rebasing isn't completely trivial and I don't want to waste effort doing it if there's no interest in the PR, which appears to be the case.

@truthbk
Copy link
Member

truthbk commented Feb 18, 2020

@njhill unfortunately this PR didn't get the attention it deserved. I apologize for the time it has taken us to provide any reasonable feedback for it.

We would be very interested in helping drive this PR forward as it would be of great value to us and the community, helping reduce both memory and cpu overhead. There's a bunch of ongoing work as well, but I'd like to give this precedence as the ongoing work will introduce some breaking changes, and trigger a new major release of the library. Also, I'd like to rebase that on top of this, and not the other way around.

I'm not sure if you'd have time to rebase, or if you'd like us to take over and finish any additional work required (all credit would still go to you, of course). Please let us know.

In any case, thank you very much for this great PR, it's very welcome and definitely of help 🙇

@njhill
Copy link
Contributor Author

njhill commented Feb 20, 2020

Thanks @truthbk, I'm not sure when I'd have time to rebase (and actually am not using the client any longer anyhow), so feel free to take that over if you need it done soon!

@truthbk
Copy link
Member

truthbk commented Feb 24, 2020

@njhill sounds good! I'll take over the PR! I might close this one and bring your changes into a branch on this repo (and not your fork). In any case your commits and authorship will be preserved. Thank you very much for this work.

@truthbk
Copy link
Member

truthbk commented Mar 11, 2020

Closing in favor of #105, thank you @njhill - rebased your contribution there.

@truthbk truthbk closed this Mar 11, 2020
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.

3 participants