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

Resolve graphite growth problem, introduce hopper 0.1.2 #181

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

blt
Copy link
Collaborator

@blt blt commented Jan 11, 2017

We noticed that there was something fishy going on with graphite.
In particular, the number of lines push to the programmable_filter
grew way too quickly for the inputs. It turns out, that fancy
line buffer we pass in to avoid allocing small strings repeatedly
was not being cleared and so everything that came in would be
appended there.

This commit also includes the work done as a part of
postmates/hopper:#5 by updating to 0.1.2. All messages will be
delivered in order--as they should have been--and hopper is now
quite a bit faster to boot as it can keep cramming in low-memory
when there's space.

Signed-off-by: Brian L. Troutwine blt@postmates.com

We noticed that there was something fishy going on with graphite.
In particular, the number of lines push to the programmable_filter
grew _way_ too quickly for the inputs. It turns out, that fancy
line buffer we pass in to avoid allocing small strings repeatedly
was not being cleared and so everything that came in would be
appended there.

This commit also includes the work done as a part of
postmates/hopper:#5 by updating to 0.1.2. All messages will be
delivered in order--as they should have been--and hopper is now
quite a bit faster to boot as it can keep cramming in low-memory
when there's space.

Signed-off-by: Brian L. Troutwine <blt@postmates.com>
@blt blt added this to the 0.5.0 - agena milestone Jan 11, 2017
@blt
Copy link
Collaborator Author

blt commented Jan 11, 2017

This resolves #181

loop {
time::delay(attempts);
match recv.next() {
None => attempts += 1,
Some(event) => {
attempts = 0;
match self.process(event) {
Ok(events) => {
for ev in events {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow..talk about nonintuitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one was tricky! This change here avoids having to continually alloc a new vector for every call to self.process, worth +/- 10ns per call. This is safe because we're calling drain on the vector. That'll remove all the values stashed in it.

But! A similar change to pass in a string buffer–which I'll point out–did not have something similar.

.map(|m| metric::Event::new_log(*m))
.chain(pyld.metrics.into_iter().map(|m| metric::Event::new_telemetry(*m)))
.collect())
for lg in pyld.logs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you describe the changes here and why they were made?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely! What this would do previously is two things:

  • alloc a new vector filed with metric::Event and
  • stash that vector inside a Result

To avoid the first step we're now passing in a pre-allocated vector res. This means we've got to explicitly push metric::Events into the vector. The successful return type is now Ok(()), which () is the unit type.

@@ -90,6 +90,7 @@ fn handle_stream(mut chans: util::Channel, tags: Arc<metric::TagMap>, stream: Tc
&mut chans,
metric::Event::Telemetry(Arc::new(Some(m))));
}
line.clear();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pulltab Here's the culprit. Without these clears the line buffer will grow and grow.

Copy link
Contributor

@pulltab pulltab left a comment

Choose a reason for hiding this comment

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

LGTM

@blt blt merged commit d748c9e into master Jan 11, 2017
@blt blt deleted the graphite_no_clear_buffer branch January 11, 2017 19:20
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.

None yet

2 participants