8000 Limit the number of rrd_create threads by ymettier · Pull Request #640 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Limit the number of rrd_create threads #640

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Limit the number of rrd_create threads #640

wants to merge 2 commits into from

Conversation

ymettier
Copy link
Contributor
@ymettier ymettier commented Jun 9, 2014

Hello,

PR #262 is a nice feature (create asynchronous threads for rrd files creation).

However, when too many new metrics come, there is still an issue : too many rrdcreate threads (I can have more than 1000 at the same time). And more there are, slower they become.

PR #244 has already proven to me that it was useful, but it does not help when the problem comes from too many RRD files to create.

So we should limit the number or create threads.

Check the attached patch and the new option CreateFilesAsyncLimit, that defaults to 0 (no limit, current behaviour) but that can be set to limit the number of threads.

CreateFilesAsync true
CreateFilesAsyncLimit 10

Description
There is a list (async_creation_list) of rrd files being created. I added a var (async_creation_list_size) with the size of this list.
Before the thread is created, async_creation_list_size is checked quickly (no lock) to see if the rrd file can be created or if there are already too many threads.
If there are too many threads, the metric is dropped and we hope that it will be sent again in 60 seconds (or whatever the interval is) and file can be created then.
This check is done fast, outside a pthread_locked section, so it is reliable only if there are too many threads.

If async_creation_list_size is small enough, the thread is created. A new check is done on async_creation_list_size because many threads can start and launch file creation at the same time (1st check was out of the pthread_locked section). The second check is reliable because it is done inside the locked section.

Note : 1st check is reliable when there are already too many threads running. This is why I implemented it : no need to start a new thread to see that it was useless.

@ymettier ymettier mentioned this pull request Jun 25, 2014
Closed
@mfournier mfournier added Patch and removed Patch labels Jul 24, 2014
@ymettier ymettier mentioned this pull request Aug 5, 2014
Merged
@octo
Copy link
Member
octo commented Sep 10, 2014

Hi Yves, thank you very much for your patch! If I read the diff correctly, then a thread is created in any case, but when async_creation_list is too long it exits immediately. Wouldn't it make more sense to check the number of threads in srrd_create_async() before creating a new thread? It would make the successful case more expensive, but I think we can afford to pick the lock up twice.

As mentioned on the commit, the memory leak as fixed in dfb341b, sorry for the duplicate work!

—octo

@ymettier
Copy link
Contributor Author

Hello,

The thread is not created in any case.

In cu_rrd_create_file() :

if (cfg->async)
{
status = srrd_create_async (filename, cfg->async_limit, stepsize, last_up,
argc, (const char **) argv);

In srrd_create_async()
1/ test if async_creation_list is too long. If yes, return. No thread.
2/ if not too long, create the thread with pthread_create(..., srrd_create_thread, ...)
3/ Inside the thread, check if the file that should be created is already being created. If yes, quit. If not, create it.

If discussion is about creating a useless thread when the queue is too long, no problem, the useless thread is not created.

A new discussion can be on locking the file inside or outside the thread.
In Collectd-5.4, the file is locked inside the thread. So my patch changes nothing at this behaviour. Because my patch has been working for weeks (months) in production environment with no problem on overloaded servers, I suggest that we have this discussion later (I would not like this patch to become obsolete because of changes in mainstream Collectd - updating patches is not my favourite job) if you don't mind. And yes, this discussion should be interesting.

Regards,
Yves

@ymettier
Copy link
Contributor Author
ymettier commented Jul 3, 2015

Hello,

It seems that this feature is implemented since Collectd-5.3 and comit 115ac52.

So I close this PR.

Thanks for implementing this feature.

Yves

@ymettier ymettier closed this Jul 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ymettier @octo @mfournier
0