-
Notifications
You must be signed in to change notification settings - Fork 52
Improving libcron performance #9
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
Improving libcron performance #9
Conversation
…orts once), renaming, adding some additional tests
…between add_schedule and the first call to tick)
…xt possible interval)
|
Nice stuff. Will have a closer look asap, a bit busy at the moment. Remind me on Thursday if I've not responded by then. |
PerMalmberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate on why you need to add multiple schedules that perform the same work instead of adjusting a single schedule? I can't see the benefit even taking multithreading into account since the tick() is still called on a single thread which means that all work is still single-threaded.
|
Hey, |
|
Ok, so I assume that most tasks have > 1s sampling interval? |
|
Actually, there are several channels sampled every second... But also Channels sampled each day at a specific time-point, channels sampled each hour, each minute, every 10 minutes..... That is what makes |
|
Ok :) We'll merge this as soon as you're done with the remaining changes. |
PerMalmberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional comments to those already not resolved.
Co-authored-by: Per Malmberg <PerMalmberg@users.noreply.github.com>
|
I think I am done now :) |
Almost :) There's still that |
|
Something like that? |
Yes! 👍 |
Hey,
And yet another pull request.
Addresses Issue
My application adds a lot of tasks to the scheduler (up to 1000) and needs quite a precise scheduling performance. Each task does not take a lot of time as the code is already optimized, however, I found out that when a lot of tasks need to be executed at the same time (say - 1000 tasks each second), the
tickcall itself becomes quite slow (especially when iterator debugging is active under MSVC, but also when executing in relase mode). Furthermore, adding 1000 tasks with the same cron-string also takes a lot of time. So I decided to investigate the issue and found the following reasons (adressed in this pull request):std::priority_queuesorts the element each time you call push. In the current implementation sorting is called for each executed task, however this is not necessary at all. Sorting can be done once at the end oftick. I addressed this issue by optimizing how often the queue is sortedstd::map<std::string, std::string>, astd::unordered_map<std::string, std::string>or astd::vector<std::pair<std::string, std::string>>toadd_schedule.CronData::create) - caching them is possible and does improve performanceTo distinguish between the added tasks when batch-adding them, I added a
get_namefunction to theTaskInformationInterface.I think we can do it the same as last time - let's discuss my approaches and if you see an improvement we can merge? In the end, I was able to get the call to tick from 700 mS for 1000 Tasks to be executed down to 1-10 mS - which is a huge improvement I guess.