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

Added period checking in RandomInput to reduce CPU load #3

Closed
wants to merge 2 commits into from

Conversation

guihomework
Copy link

Solves issue #2 according to suggestions

Copy link
Member

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Instead of low-level C routines, consider using std::chrono::steady_clock. I think you won't need the TimeUtils.* files then.

Comment on lines 26 to 29
#include <stdint.h> // `UINT64_MAX`
#include <unistd.h>
#include <stdio.h> // `printf()`
#include <time.h> // `timespec_get()`
Copy link
Member

Choose a reason for hiding this comment

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

Don't pollute headers with not required includes. Move them to the .cpp.

@guihomework
Copy link
Author

Instead of low-level C routines, consider using std::chrono::steady_clock. I think you won't need the TimeUtils.* files then.

I knew you would ask that, but first the whole app is using a lot of C code everywhere and second the problem is not on measuring the time, but on waiting.
Hence I tested if this recent std::chrono would be better than the C routine, but it isn't according to my tests see #2 (comment) (does not add to the precision, the c-routing can get), so I did not see the need to switch the whole code to modern c++ just to use std::chrono if it does not solve the issue of precise timing.

I do not have time to modernize this app now (and also not the skills as you know) but tried std::chrono as I showed
I only wanted to avoid heating up my laptop and ended-up already spending far too much time with this imprecise timing that I tried to solve.

So I would rather keep this old-style code to not have mixed old-style modern c++

@rhaschke
Copy link
Member

As you already used std::chrono in your test, you proved that you are capable of using it 😉
Sure, it doesn't solve the timing precision, but it produces more readable code. Yes, I am aware that this will mix old-style C code (dating back to Matthias) with modern C++ code. But, why should we stick to old-style when introducing new code?
At some point we need to migrate...

@guihomework
Copy link
Author

The question is not of being capable or not. Finding examples for a tiny modern c++ solution is always possible but changing the whole app to something suitable is another task that I do not feel capable of doing (and I repeat I do not have time to do it)

Mixing is something I did not know you were ready to accept is also one reason I did not provide a PR with this std::chrono in the first place.

I stay on the impression that this std::chrono is more complex than simpler time struct well known for so many years and more explicit. For instance I do not understand how a duration needs a .count() to extract integers out of the time struct. Not intuitive at all for me who never worked with it.

so if you do not want this PR as is, close it please. I will keep the changes on my fork only. I do not have time to modernize the code.
Sorry.

@rhaschke
Copy link
Member

rhaschke commented Sep 1, 2022

Superseeded by #4

@rhaschke rhaschke closed this Sep 1, 2022
@guihomework guihomework deleted the period_for_random branch September 6, 2022 15:40
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