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

fix: remove log message bomb #83

Closed
wants to merge 1 commit into from

Conversation

moazzamak
Copy link

Removes message bomb that happens if a frame fails to capture for whatever reason.
It was generating 1.5 Gb/sec on the rosbag and equally as much in the ~/.ros/log folder.

@awesomebytes
Copy link
Member

Hello, first, thanks for the contribution!

If it's just a matter of a problem of logging too fast, I'd probably go with changing the two logs to:

NODELET_ERROR_STREAM_THROTTLE(1.0, 
NODELET_WARN_STREAM_THROTTLE(1.0, 

Once a second seems reasonable to me.

If this also hogs the CPU (which I could see it happening), I do like your workaround, as the sleep only happens when the read didn't work. What do you think? Did you experience a lot of CPU usage when this happened?

@moazzamak
Copy link
Author

Hi,
It definitely hogs the CPU. My system can somewhat manage it since it's a pretty beefy quad core but I imagine smaller systems might be affected more.
I believe this fix may solve the "memory leaks" reported in other issues as well since the messages may end up overflowing the buffers and this would impact overall memory available if you don't specify a small buffer size for rosbag recorder.

@moazzamak
Copy link
Author

Is this project dead?

@awesomebytes
Copy link
Member

awesomebytes commented Dec 11, 2020 via email

@moazzamak
Copy link
Author

bump

@awesomebytes
Copy link
Member

Closing, as I reimplemented it with the throttling here: #96

Sorry for the lack of time! I finally finished my PhD and I have some time :)

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.

2 participants