Ok, so I have the opposite problem - too meany threads make it really easy to exhaust some limited resources.  Namely, file descriptors.

Concretely in ostree, I am a heavy user of asynchronous GIO.  Things like GFileEnumerator hold a fd (indirectly via DIR*).  It's actually incredibly easy for me to bump up against the Linux default of 1024 descriptors when you start doing stuff like recursive directory traversal and copying things around, and the recent commit to go from 10 worker threads to 100 made it much easier to hit.

(If you want to reproduce, I have a not-merged branch that changes "ostree pull-local" to use libsoup with file:// URIs; also, my https://github.com/cgwalters/glib-async-rm-rf hit it too)

There's been some discussion of changing the Linux default to 4096;
https://bugzilla.kernel.org/show_bug.cgi?id=32382

But even that is still just prolonging the problem; this is really a question of resource scheduling.

I have a proposed solution that likely everyone here will think WTF! but hear me out:

When we start GLib, we inspect the maximum fd count for the current process.  We then choose a maximum percentage of that to consume (say 80%), leaving the rest for non-GLib applications.

Now inside GLib, everywhere we call open() or otherwise get a file descriptor, we check for EMFILE.  If we hit it, we block on a condition variable.  Everywhere in GLib that we call close(), we signal on the condition.

Any other ideas?

Er, scratch the EMFILE bit.  We need to keep an internal counter up to our maximum fd percentage.

An alternative (or likely, we want both) approach is to essentially switch to a "pull" model for tasks.  We could have say:

gulong g_async_add_creator (GSourceFunc cb, gpointer user_data, GDestroyNotify notify);
void g_async_remove_creator (gulong id);

The callback would fire when the system wants to request a task.  Writing applications using this would be quite different though...


Yet another alternative is for tasks to explicitly inform the system when they've queued any dependent tasks, and are about to block on a system call or the like.  This would change the squence for the Evolution case above to look like [image] [dns] [image] [dns] [image] [dns] which is obviously better for many reasons.


However, I think for maximum compatibility, the file descriptor hack is going to be necessary.

(In reply to comment #3)
> However, I think for maximum compatibility, the file descriptor hack is going
> to be necessary.

That is mostly a different bug though.

(In reply to comment #1)
> Now inside GLib, everywhere we call open() or otherwise get a file descriptor,
> we check for EMFILE.  If we hit it, we block on a condition variable. 
> Everywhere in GLib that we call close(), we signal on the condition.

We can't just block; tons of places assume that open(), socket(), pipe(), etc will never block. You'd have to add async versions of those APIs. (Or have a FileDescriptorsAvailableSource they can wait on.)

(In reply to comment #3)
> Yet another alternative is for tasks to explicitly inform the system when
> they've queued any dependent tasks, and are about to block on a system call or
> the like.

Yeah, I've thought about that a little too...

(In reply to comment #4)

> We can't just block; tons of places assume that open(), socket(), pipe(), etc
> will never block. You'd have to add async versions of those APIs. (Or have a
> FileDescriptorsAvailableSource they can wait on.)

I'm going to make a broad claim: there exist no GLib-using applications that handle EMFILE currently in any sensible way.  You can't really, since some places inside GLib just call g_error() and stumble on (e.g. return NULL) if this happens.  For example, GWakeup.

Remember that the blocking we're talking about here will only happen when the process is nearing the max file descriptor count.  We could also even make it conditional on being inside a GTask worker thread (use a GPrivate key _g_is_in_gtask ()).

(In reply to comment #3)
> Yet another alternative is for tasks to explicitly inform the system when
> they've queued any dependent tasks, and are about to block on a system call or
> the like.  This would change the squence for the Evolution case above to look
> like [image] [dns] [image] [dns] [image] [dns] which is obviously better for
> many reasons.

If I got this right, then you mean a serialized requests. I do not think it's the way to go at all. The main advantage of going to WebKit render in evolution was to avoid slow image downloads, with which WebKit helps quite well.

All the above seems to me that you want to stick with some pretty fresh GTask, which, obviously, doesn't server to its needs. But that's just my impression from the above, I do not know the internals and such.

(In reply to comment #6)
> (In reply to comment #3)
> > Yet another alternative is for tasks to explicitly inform the system when
> > they've queued any dependent tasks, and are about to block on a system call or
> > the like.  This would change the squence for the Evolution case above to look
> > like [image] [dns] [image] [dns] [image] [dns] which is obviously better for
> > many reasons.
> 
> If I got this right, then you mean a serialized requests. 

No; if the tasks were truly serialized, the above would never finish.  The proposal is simply a more optimal scheduling.

Ok, so I spent a bit of time this morning doing background research on how major real-world projects deal with async-io.  Namely, I studied QEMU.

First interesting thing to note - they link to GLib!  But just for the mainloop integration and use g_malloc()/g_free().

Looking more closely, it turns out that QEMU mostly uses the POSIX AIO framework.  There's some kernel support for this, but from what I can tell in the glibc sources, the way it basically works is that it's centered around reading/writing file descriptors, using threads.

http://www.kernel.org/doc/man-pages/online/pages/man7/aio.7.html

Unfortunately there's a limit to how much we can/should emulate this, since we have a much more general async API (enumerate directories, rename files, etc.).  There is one bit that's interesting - at least the glibc implementation defaults to using 20 threads, but it's tunable (globally) via aio_init().  How did they arrive at 20?  Who knows...  From what I can tell, QEMU does not override the default of 20.

Created attachment 231522 [details] [review]
WIP: GTask kinds

My application (OSTree) has suffered really bad performance
degredation recently, and doing some analysis of the problem, I think
a lot stems from the recent order of magnitude increase in the default
GLib worker threads.

OSTree does a *lot* of async I/O (directory enumeration, calling
link(), reading/writing files) etc.  It very quickly reaches 100
threads, but there is up/down thread churn as threads complete, but
process gets I/O bound and thus the threads are starved of work, but
then the main thread manages to fill the queue more, suddenly spinning
up lots of worker threads again.

Now, as this patch forces users to specify, there are several distinct
classes of tasks.  It basically never makes sense to run more
CPU-bound threads than there are CPUs.  Spawning 90+ threads to do
SHA256 calculation for example is just creating pointless extra
contention on a dual-processor system.

Therefore, the limit of the thread pool for G_TASK_THREAD_KIND_CPU is
the number of processors.

Similarly, there's a real limit to how much I/O traffic it makes sense
to schedule simultaneously.  I need to do more research here - what
makes sense for my laptop with 1 magnetic hard drive is likely
different from a fast server with RAID on top of SSDs.  For the
moment, I've chosen to limit I/O bound threads to 4.

The limit for _DEFAULT remains 100 - but we can solve this better if
we have e.g. a way for tasks to express dependencies, among other things.

But for now, with this patch, and OSTree modified to use
G_TASK_THREAD_KIND_IO for its rename()/link() threads, I am seeing
better performance.

I wish I could get reliable benchmarks here - but there are incredibly wild swings.  First, OSTree is heavily sensitive to warm vs cold cache.  Running an operation the second time can go from a minute to 5 seconds or less.

Secondly, I have a theory that the scheduling of the main thread is a key factor.  In some few cases I do blocking I/O in the main thread, and if that happens to get slotted behind the workers, it becomes rather catastrophic as the majority of threads time out, starved of work.  Then the main thread comes back, pushes lots of work, spinning up the pool, but then gets starved, and the process repeats.

Also obviously, a limit of 4 G_TASK_THREAD_KIND_IO fixes my "hitting file descriptor limit" problem.

Unfortunately at the moment, most of GLib is not ported to GTask; namely, GFileEnumerator's next_files_async is what OSTree is a heavy, heavy user of.  So we still end up spawning 4 + (N < 100) threads which, but decreasing N is definitely better for me.


Anyways, operating under the assumption that we have _CPU and _IO threads, let's try to chip away at the _DEFAULT case.  The core problem that started this bug is the pattern of:

async -> thread -> async -> thread.

Ok so Dan - can you explain to me - why does g_task_run_in_thread_sync() exist?  Why don't we just do the operation synchronously in the *current* thread, instead of spawning an entirely new one and just blocking on it?

(In reply to comment #12)
> Unfortunately at the moment, most of GLib is not ported to GTask

I would say _fortunately_, because I understand this bug as a regression with introduction of GTask - there was no such issue in 2.34.x-, because there was no GTask. It might sound rough, but that's my impression.

(In reply to comment #9)
> OSTree does a *lot* of async I/O (directory enumeration, calling
> link(), reading/writing files) etc.  It very quickly reaches 100
> threads

The current "100 thread" thing is a *complete hack*, and if this patch is only intended to deal with that, then no. (And if evolution were to start using non-DEFAULT thread kinds, then it would just bring back its previous deadlock.)

OTOH, the patch might make sense independent of that problem? I'd have a slight preference for "g_task_set_kind()" rather than changing the existing API though...

(In reply to comment #12)
> Unfortunately at the moment, most of GLib is not ported to GTask

I've rebased and re-pushed wip/task. I did some sanity checking of the I/O and VFS patches, and fixed a few bugs, but haven't tested it heavily (other than "make check" in gio) yet. But we could land it after next week's release maybe?

(In reply to comment #13)
> I would say _fortunately_, because I understand this bug as a regression with
> introduction of GTask

It's unfortunate because g_simple_async_result_run_in_thread() uses GTask internally now, so it is subject to the GTask bugs (both the original deadlock and the new too-many-threads) but wouldn't be able to use walters's suggested new API to fix it.

We could revert that part of the GTask porting though (the gioscheduler.c part of 55e7ca6), if we don't have a fix coming soon...

> let's try to chip away at the _DEFAULT case.  The core problem that started
> this bug is the pattern of:
> 
> async -> thread -> async -> thread.
> 
> Ok so Dan - can you explain to me - why does g_task_run_in_thread_sync() exist?
>  Why don't we just do the operation synchronously in the *current* thread,
> instead of spawning an entirely new one and just blocking on it?

Because our API needs to be cancellable, but the underlying operation (getaddrinfo()) isn't cancellable.

But maybe we should just address this one specific case; if you run g_task_run_in_thread_sync() from inside a GTask thread, it should just bump up the max thread count for the duration? Or perhaps, we could have a separate GThreadPool of unlimited size that was only used for running tasks-started-inside-other-tasks?

(In reply to comment #14)
> (In reply to comment #9)
> > OSTree does a *lot* of async I/O (directory enumeration, calling
> > link(), reading/writing files) etc.  It very quickly reaches 100
> > threads
> 
> The current "100 thread" thing is a *complete hack*, and if this patch is only
> intended to deal with that, then no. 

There are a lot of use cases and problems here.  The Evolution case is one which it's true this patch does not fix.

> (And if evolution were to start using
> non-DEFAULT thread kinds, then it would just bring back its previous deadlock.)

Yes, it shouldn't though.

> OTOH, the patch might make sense independent of that problem? 

I think so - for cases like I have in OSTree where I do SHA256 calculation as an async op, doing 100 threads of that is insane.  Similarly, I don't want 100 threads calling rename() or readdir().

> I'd have a slight
> preference for "g_task_set_kind()" rather than changing the existing API
> though...

Yeah...makes sense.  Will redo the patch.

> But maybe we should just address this one specific case; if you run
> g_task_run_in_thread_sync() from inside a GTask thread, it should just bump up
> the max thread count for the duration? Or perhaps, we could have a separate
> GThreadPool of unlimited size that was only used for running
> tasks-started-inside-other-tasks?

Why can't we handle the "cancelled uncancellable task" case by *only* spawning another thread if we detect that's happened?

Created attachment 231573 [details] [review]
Add GTask kinds

Use _set_kind() for now.  However, I think in the future, I'd like to
force GTask users to specify the kind.  Otherwise it's too easy for
people to miss.

(In reply to comment #15)
> > OTOH, the patch might make sense independent of that problem? 
> 
> I think so - for cases like I have in OSTree where I do SHA256 calculation as
> an async op, doing 100 threads of that is insane.  Similarly, I don't want 100
> threads calling rename() or readdir().

But that's why I said "independent of this problem"; the 100 threads is just a nasty temporary hack that will go away as soon as we've solved the deadlock issue. The question isn't "are task kinds useful when max-threads is 100?", it's "are task kinds useful when max-threads is 10?". (The answer may be yes, I don't know.)

> Why can't we handle the "cancelled uncancellable task" case by *only* spawning
> another thread if we detect that's happened?

When we detect what has happened? The cancellable has been cancelled? What would we do in that thread in that case?

The goal is to have g_resolver_lookup_name() return immediately-ish when the cancellable gets cancelled. But there's no way to interrupt a getaddrinfo() call, so if g_resolver_lookup_by_name() were calling getaddrinfo() directly, there'd be no way it could ever return early. So instead, it runs getaddrinfo() in another thread, and then g_task_run_in_thread_sync() waits until either the getaddrinfo() thread finishes, or the GCancellable gets cancelled, and then it returns. (In the cancellation case, the getaddrinfo() thread keeps running after g_resolver_lookup_by_name() returns, and when it eventually finishes, GTask takes care of throwing away its return value.)

(In reply to comment #17)
> The question isn't "are task kinds useful when max-threads is 100?",
> it's "are task kinds useful when max-threads is 10?". (The answer may be yes, I
> don't know.)

Ok, fair enough, but even were we to go back to the 2.34 of "10 max threads", it's still quite lame, though not as catastrophic as 100.  On a machine with just two cores (modern smartphones) spawning 10 worker threads for CPU bound tasks is just burning power.  It creates additional lock contention, CPU cache thrashing, etc etc.

> The goal is to have g_resolver_lookup_name() return immediately-ish when the
> cancellable gets cancelled. But there's no way to interrupt a getaddrinfo()
> call, so if g_resolver_lookup_by_name() were calling getaddrinfo() directly,
> there'd be no way it could ever return early. So instead, it runs getaddrinfo()
> in another thread, and then g_task_run_in_thread_sync() waits until either the
> getaddrinfo() thread finishes, or the GCancellable gets cancelled, and then it
> returns. (In the cancellation case, the getaddrinfo() thread keeps running
> after g_resolver_lookup_by_name() returns, and when it eventually finishes,
> GTask takes care of throwing away its return value.)

Right, I understand now, sorry about it taking a few times to get through.  I guess we just have to accept these "uncancellable system call threads" as the price to pay for not hacking glibc to give us better APIs.

(git bz isn't working for me on RHEL6 at the moment)

I pushed two commits two wip/task:

http://git.gnome.org/browse/glib/commit/?h=wip/task&id=a676dec260563d844fbc8529e859d693900e35793

I'm much happier with this latest iteration, now g_task_set_scheduling() since we're not adding another line of code to each call site.

Comment on attachment 231573 [details] [review]
Add GTask kinds

See wip/task

Created attachment 231625 [details] [review]
gtask: don't deadlock when tasks block on other tasks

If tasks block waiting for other tasks to complete then the system can
end up starved for threads. Avoid this by bumping up max-threads in
that case.

Review of attachment 231625 [details] [review]:

::: gio/gtask.c
@@ +1315,3 @@
+       */
+      if (g_thread_pool_set_max_threads (task_pool,
+                                         g_thread_pool_get_max_threads (task_pool) + 1,

Isn't this racy if two threads start tasks at the same time?

Created attachment 231715 [details] [review]
gtask: don't deadlock when tasks block on other tasks

> Isn't this racy if two threads start tasks at the same time?

Oops, yes. Fixed.

(In reply to comment #19)
> I'm much happier with this latest iteration, now g_task_set_scheduling() since
> we're not adding another line of code to each call site.

ah, nice. (Though... I might be convinced that we want to add the argument to g_task_run_in_thread(), to force people to think about it?)

gio.symbols still has "g_task_set_kind" (and docs/ doesn't have anything)

The GTaskThreadKind docs have "G_TASK_THREAD_KIND_LOCAL_IO", but the enum value is just "G_TASK_THREAD_KIND_IO". Though network I/O should be probably in a different resource-usage bucket than disk I/O, right? (Although I guess GInputStream and GFile mostly don't actually know which kind they're doing, so maybe not...)

Maybe change the variable names from "kind" to "thread_kind"? There might be other kinds of kinds later...

>+ * This function must be used before invoking g_task_run_in_thread()
>+ * or similar.

That can be parsed as "you always need to call this" rather than "you need to call this first". "If you are going to use this function, you must..." ? Clunky, but...

The g_task_run_in_thread() docs still include @kind even though the function doesn't.

>+get_nproc_onln (void)

That's some classic old-school UNIX function naming there. :)

See bug 614930 which adds g_get_num_processors() with BSD and Windows support too.

>+  task_pool_default = generic_pool_new (max_default_threads);
>+  task_pool_cpu = generic_pool_new (nproc);
>+  task_pool_io = generic_pool_new (max_io_threads);
>+  task_pool_mixed = generic_pool_new (nproc + max_io_threads);

Ideally task_pool_cpu wouldn't use all nproc threads if the other pools were non-empty... doesn't need to be fixed now, but maybe worth a comment at least.

> g_task_thread_pool_resort (void)
> {
>-  g_thread_pool_set_sort_function (task_pool, g_task_compare_priority, NULL);
>+  g_thread_pool_set_sort_function (task_pool_default, g_task_compare_priority, NULL);

It needs to either re-sort all 4 pools or else take a GThreadPool argument.

>+void          g_task_set_scheduling        (GTask               *task,
>+                                            gint                 glib_priority,

"glib_priority" doesn't match the function definition (or the other functions)

Review of attachment 231715 [details] [review]:

Ok, this makes sense to me.

You should mention in your commit message that you're flipping the default for non-dependent threads back to 10.

For my issue, see also:

https://bugzilla.gnome.org/show_bug.cgi?id=501237

If we can schedule worker threads with a lower priority, I think that would help avoid the ping-pong I was seeing with the queue before.

The tricky thing about this is that it's a (very minor/subtle) ABI break for g_thread_pool if we just do it by default for the threads we spawn from non-exclusive pools.  But honestly, I wonder if anyone would actually have a problem with it.

Dan, is there a bug for the rest of the wip/task porting to land?  What's blocking it?

(In reply to comment #14)
> It's unfortunate because g_simple_async_result_run_in_thread() uses GTask
> internally now, so it is subject to the GTask bugs (both the original deadlock
> and the new too-many-threads) but wouldn't be able to use walters's suggested
> new API to fix it.

(In reply to comment #24)
> ah, nice. (Though... I might be convinced that we want to add the argument to
> g_task_run_in_thread(), to force people to think about it?)

Do the two above imply that you'll change API for g_simple_async_result_run_in_thread(), to force people think? And then to each and every GLib function which will internally use GTask, which is an information I definitely do not want to think/know of at all?

It still feels like a bad regression in GTask design/implementation/usage. You've an advantage that it was discovered early so it still can be "made working". Maybe I'm the only one, but I do not have a good feeling about GTask (which you might understood from my comments here).

(In reply to comment #28)
> (In reply to comment #14)
> > It's unfortunate because g_simple_async_result_run_in_thread() uses GTask
> > internally now, so it is subject to the GTask bugs (both the original deadlock
> > and the new too-many-threads) but wouldn't be able to use walters's suggested
> > new API to fix it.
> 
> (In reply to comment #24)
> > ah, nice. (Though... I might be convinced that we want to add the argument to
> > g_task_run_in_thread(), to force people to think about it?)
> 
> Do the two above imply that you'll change API for
> g_simple_async_result_run_in_thread(),

No, that's stable API, we can't change it.

> It still feels like a bad regression in GTask design/implementation/usage.
> You've an advantage that it was discovered early so it still can be "made
> working". Maybe I'm the only one, but I do not have a good feeling about GTask
> (which you might understood from my comments here).

I don't understand what you're complaining about, honestly.  What's the concrete problem?

(In reply to comment #27)
> Dan, is there a bug for the rest of the wip/task porting to land?  What's
> blocking it?

No... what's blocking it is mostly this bug. I'll land the next batch (I/O and vfs stuff) after this is fixed.

(In reply to comment #28)
> Do the two above imply that you'll change API for
> g_simple_async_result_run_in_thread()

no

> And then to each
> and every GLib function which will internally use GTask, which is an
> information I definitely do not want to think/know of at all?

No, the function creating the task knows what the task will be doing. Eg, GFile would pass "G_TASK_THREAD_KIND_IO" for all of its ops.

> It still feels like a bad regression in GTask design/implementation/usage.

The thread kind stuff is completely unrelated to the deadlock and to GTask. It's something where gio has always been a little bit inefficient in its use of threads in some cases, and the *workaround* that was introduced for the deadlock bug made the minor inefficiency become major in those cases. (Hence the discussion around comments 17 and 18 about whether we still would want thread kinds after reverting the workaround.)

And the deadlock is not particularly related to GTask either; exactly the same deadlock would have shown up if GResolver had been ported to use g_simple_async_result_run_in_thread() (pre-GTask GResolver used GThreadPool directly rather than using g_simple_async_result_run_in_thread(), for reasons that made sense in glib 2.22 but later became irrelevant.)

Created attachment 231828 [details] [review]
Updated patch

* Fixed up for comments
* Now uses g_get_num_processors()

Created attachment 231829 [details] [review]
GTask-Scale-threads-to-number-of-processors.patch

Thinking about this a lot more...I like this (obviously much simpler) patch better for now.  But leaving the other one un-obsoleted for comparison.

Ok, let me go back to the high level here.  There are several goals:

1) Correctness: We should not hang in cases like the image rendering + DNS one.
2) Responsiveness/Resourcing: Avoid overloading the system.  For example, don't make the main thread fight for scheduling.  Don't load 1000 file descriptors and potentially crash the app.
3) Performance.


Now, 1) is fixed, and should remain fixed.  

2) is still an outstanding issue.  For example, if we take my last patch (and I think we should), we potentially are back in the danger zone for file descriptors on 64 processor boxes.  I don't have any great ideas for this at the moment.  Ideally the kernel could help us here.

3) is a lesser concern - but I think it'd be cool if ostree lit up all 64 processors on one of those machines =)

But let's remain focused on 2).

Comment on attachment 231828 [details] [review]
Updated patch

>+ * @G_TASK_THREAD_KIND_IO: Performs synchronous I/O

I'd remove "synchronous"; if you for some reason want to do async I/O in a thread, it will have the same performance characteristics, it just changes which syscall you block in.

>-  else if (g_private_get (&task_private))
>+  else if (pool == task_pool_default && g_private_get (&task_private))
>     {
>       /* This thread is being spawned from another GTask thread, so
>        * bump up max-threads so we don't starve.

You can't limit this to only pool_default. The other pools have the same problem. Anyway, easy fix; just replace blocking_other_task with a GThreadPool variable saying which one you bumped up.

Likewise the comment from before about resort() not applying to only pool_default still applies.

(While reading this patch I wonder if it would make more sense to still have only a single pool, but use task->kind in the sort function... This would require more cleverness out of GThreadPool though, since we'd want to be using more threads when all the tasks were IO than when they were CPU.)

>+  /* G_TASK_THREAD_KIND_DEFAULT: Why 10?  See
>+   * https://bugzilla.gnome.org/show_bug.cgi?id=687223

This bug doesn't actually explain why 10. (The answer is "because that's what GIOScheduler used...". OK, I guess now it does explain why 10. :-)

Comment on attachment 231829 [details] [review]
GTask-Scale-threads-to-number-of-processors.patch

>   task_pool = g_thread_pool_new (g_task_thread_pool_thread, NULL,
>-                                 10, FALSE, NULL);
>+                                 g_get_num_processors () + 1, FALSE, NULL);

I suppose it makes sense to have it be based on num_processors in some way, but N+1 seems too low unless you expect that most threads will be CPU-bound...

(In reply to comment #35)
> (From update of attachment 231829 [details] [review])
> >   task_pool = g_thread_pool_new (g_task_thread_pool_thread, NULL,
> >-                                 10, FALSE, NULL);
> >+                                 g_get_num_processors () + 1, FALSE, NULL);
> 
> I suppose it makes sense to have it be based on num_processors in some way, but
> N+1 seems too low unless you expect that most threads will be CPU-bound...

Why is it too low?  As a very broad rule, I think there's a correlation between number of processors and I/O speed, for example.  64 processor boxes are likely to have fast RAID 10k RPM drives, so it's possible it makes sense to spawn more I/O worker threads too.  The correlation for uniprocessor systems is more varied I expect.

However, the DNS worker thread is a special case I think, where the threads are just parked waiting on the network, we're not imposing load on the host system.  Maybe we can add a _g_task_run_in_thread_with_flags (_G_TASK_FLAG_FORCE_NEW_THREAD)?

The difference between this an my first patch is that it's not public API; I think GLib should handle wrapping most system calls like this, and applications won't need to do so.

Created attachment 232017 [details] [review]
0001-Add-private-GTask-API-to-force-running-in-thread-use.patch

Patch on top of previous one, allows GThreadedResolver to always create new threads.

So did you know that calling g_thread_pool_set_max_threads() actually spawns immediately that maximum number?  So even currently with the max of 10, when we spawn just one thread to say do a DNS lookup, 9 other pointless threads are spawned too...

Honestly - GThreadPool is a pile of crap.  It was clearly taken from Java, but we don't have the same needs/limitations as the JVM.  We have mainloops, not a thread per HTTP connection.

(In reply to comment #38)
> So did you know that calling g_thread_pool_set_max_threads() actually spawns
> immediately that maximum number?

Only for exclusive thread pools. Which really, there's no reason for anyone to ever use. (Nothing in glib does except for test cases.) I guess the idea is that they behave like apache, where you want to have all the threads/processes ready and waiting from the start so you don't have to waste time starting them up later.

Although g_thread_pool_set_max_threads() seems pretty broken for non-exclusive thread-pools too; it always spawns as many threads as would be needed to immediately run all tasks currently waiting to be started on that pool, even though the scheduler won't actually use more than the new value of max-threads of them... (IOW, it appears to have the right semantics from the outside, but it also gratuitously spawns extra threads that it can't make use of in some cases).

(In reply to comment #36)
> However, the DNS worker thread is a special case I think, where the threads are
> just parked waiting on the network, we're not imposing load on the host system.
>  Maybe we can add a _g_task_run_in_thread_with_flags
> (_G_TASK_FLAG_FORCE_NEW_THREAD)?

I don't think they're that special a case; most network I/O is going to be like that. Seems like we just want G_TASK_THREAD_KIND_NETWORK.

(In reply to comment #40)
> (In reply to comment #36)
> > However, the DNS worker thread is a special case I think, where the threads are
> > just parked waiting on the network, we're not imposing load on the host system.
> >  Maybe we can add a _g_task_run_in_thread_with_flags
> > (_G_TASK_FLAG_FORCE_NEW_THREAD)?
> 
> I don't think they're that special a case; most network I/O is going to be like
> that. Seems like we just want G_TASK_THREAD_KIND_NETWORK.

Are you arguing for the public API or the private one?

public. an addition to the existing thread kinds patch

Created attachment 232093 [details] [review]
0001-Add-private-GTask-API-to-set-scheduling-use-it-in-re.patch

Ok, for now, can we keep this API private for now so it has time to bake?  

One thing that's on my mind is that it might be good to allow associating IO tasks with a file descriptor or file path.  For example, in the future the kernel might be able to help us estimate how many threads we should spawn for output to a given block device.

At the moment the API just special cases _NETWORK - _CPU and _IO are treated the same.

Review of attachment 232093 [details] [review]:

Although this is much simpler, I like the multiple-pools approach from before a little better. You don't really want to allow an *unlimited* number of simultaneous DNS resolutions, since that can cause other problems (https://bugs.webkit.org/show_bug.cgi?id=41630#c0. Basically, if the upstream DNS server can't answer your DNS requests as fast as you're sending them, then eventually you're going to hit getaddrinfo()'s internal timeout).

Maybe we need an API more like davidz suggested in bug 661767 comment 57, where you can create your own "GTaskThreadPool" with its own parameters, which might include a pool-specific max-threads ("you can only do N concurrent DNS requests, regardless of what else is going on") as well as something like GTaskScheduling to help coordinate usage between different pools ("you can only have 4 concurrent threads from all G_TASK_SCHEDULING_IO pools"). I'd do something like g_task_set_pool() rather than pushing a thread-default like in his example though.

::: gio/gtask-private.h
@@ +34,3 @@
+} GTaskScheduling;
+
+void          _g_task_set_scheduling (GTask           *task,

the problem with _-prefixed names is that you'll have to change every call site if we do want to make this public later...

::: gio/gtask.c
@@ +1321,3 @@
     {
       /* This thread is being spawned from another GTask thread, so
        * bump up max-threads so we don't starve.

comment would need to be updated to explain both cases (or at least, to not imply that both cases are the same)

(In reply to comment #44)
> Review of attachment 232093 [details] [review]:
> 
> Although this is much simpler, I like the multiple-pools approach from before a
> little better. You don't really want to allow an *unlimited* number of
> simultaneous DNS resolutions, since that can cause other problems
> (https://bugs.webkit.org/show_bug.cgi?id=41630#c0. 

Interesting problem.  However, we're not allowing unlimited, merely up to the CPU count.  Granted there's only a weak at best correspondence between CPU count and number of DNS requests we should initiate, but it's also better than 10.

> +void          _g_task_set_scheduling (GTask           *task,
> 
> the problem with _-prefixed names is that you'll have to change every call site
> if we do want to make this public later...

Right, but until we land some form of bug 688681 I like the low-tech _ prefixing to avoid symbol leakage.

> ::: gio/gtask.c
> @@ +1321,3 @@
>      {
>        /* This thread is being spawned from another GTask thread, so
>         * bump up max-threads so we don't starve.
> 
> comment would need to be updated to explain both cases (or at least, to not
> imply that both cases are the same)

Ok.

So...I am fearing a bit trying to solve the world with this patch.  Now that we're back down to 10 threads it's not a *burning* issue for me anymore, but it'd be nice to land this small patch I think and we can come back to doing more here later.

Just the comment fix OK?

(In reply to comment #45)
> Interesting problem.  However, we're not allowing unlimited, merely up to the
> CPU count.

No, with just the CPU-count patch you'd have that behavior, but with this patch, every time a G_TASK_SCHEDULE_NETWORK task gets queued, max-threads gets incremented, so there's no upper limit.

> Granted there's only a weak at best correspondence between CPU
> count and number of DNS requests we should initiate, but it's also better than
> 10.

(FWIW, 10 is fine. It's just that pre-GTask, GThreadedResolver had no max-threads, so it could get up to like 100 when WebKit was pre-resolving a whole bunch of links.)

*** Bug 691208 has been marked as a duplicate of this bug. ***

Note this is still a problem for me:

https://bugzilla.gnome.org/show_bug.cgi?id=706380

How exactly is it still a problem? There should be a 10-thread limit now

(In reply to comment #49)
> How exactly is it still a problem? There should be a 10-thread limit now

Think of the case of recursive asynchronous directory traversal - as the hierarchy gets deeper, you use more file descriptors for parent directory enumerators.

This is hard for applications to control or work around, short of heuristically and arbitrarily limiting parallelism.  What would be nice is if GLib just automatically tuned down the thread count/task usage as I started using more file descriptors.

Created attachment 272230 [details] [review]
threadpool: Add API to set a maximum waiting time

I've been hitting this problem, I'm suggesting adding a timeout, if the thread pool hasn't processed anything by that time this timeout expires, then create a new thread, this will never allow starvation.

Also use it in the GTask threadpool because that's where I'm hitting it.

Comment on attachment 272230 [details] [review]
threadpool: Add API to set a maximum waiting time

Could you give more details about the problems you're running into? How/where is it getting stuck?

The general idea seems sound though.

Quick review:

>+  retval->max_wait_time = 100;

If the default value isn't going to be 0, the docs should note that

>+ * @max_wait_time: Maximum time wait time in in milliseconds

I think we're trying to be consistent about using microseconds for new time-related APIs, even when you don't need that level of accuracy, just for consistency's sake.

>+ * If this is set to a value other than 0, then more thread than the

"more threadS"

>+ * maximum number of threads will be spawned no thread is available

"spawned IF no thread"

>+ * Since: 2.40

Although glib is technically not subject to the GNOME 3.12 code freeze, it still seems way too late to add new API. So this is probably for 2.42.

The usecase is simple, I have a test app that creates 100 GTls connections to itself as fast as possible (to try to find races & locking problems). Unless I slow it down a lot, the whole pool gets taken up by the client handshake threads so it can never create the server handshake threads and can never connect.

Created attachment 272694 [details] [review]
threadpool: Add API to set a maximum waiting time

The 100 ms was a leftover from testing, I also fixed the english.

There is already g_thread_pool_get/set_max_idle_time() that's in
milliseconds, so I tried to be consistent with that.

I put 2.40 because there is no GLIB_AVAILABLE_IN_NEXT macro, I guess whoever
commits it get to pick the right version.

So this problem is hitting evolution again; when connecting to a caldav server, g_task_run_in_thread_sync() gets run and spawns a thread to authenticate to the server. But that thread needs to get a lock which is held by another thread, which needs to call g_task_run_in_thread_sync() (either to do a DNS lookup or a TLS handshake) while it's holding the lock. So you have a GTask thread blocking on a thread that starts another GTask thread, and GTask can't see the whole picture, so it doesn't know to bump up the thread count, and so if you try to connect to too many servers at once, you hit the 10-thread limit and it gets stuck.


I don't think there is any way for GTask to rate-limit tasks entirely on its own without causing deadlock/starvation in some scenario.

I'm also not convinced that this can be fixed by having GTask "kinds" or pools like we'd discussed above; in many (most?) cases, the code that eventually calls g_task_run_in_thread*() is too far away from the code that is queuing the jobs, and so would not be able to assign kinds/pools correctly. (Eg, in the evolution case, half of the tasks are being spawned by e-d-s directly, but half are being spawned from within glib from within libsoup.)

So I think the only fix is for GTask to stop doing rate limiting, and say that higher-level code that might be spawning too many tasks needs to do the rate limiting itself.


We can't just suddenly drop the thread limit, since that would bring back Colin's ostree EMFILE bug (comment 1), the WebKit DNS-preresolving bug (comment 44), etc. But what if we do something like Olivier's patch above (allowing more threads to be spawned if we remain "stuck" at max-threads for too long), except instead of with a constant delay, have an initially-small, but exponentially-increasing delay. This would immediately fix the evolution problem (although making things run slightly more slowly than they actually need to), without bringing back the ostree/WebKit problems (given a suitable exponential backoff).

And then, in each GLib release for the next few years, we would shrink the exponential increase a little bit more, until eventually there is no longer any rate limiting occurring. This would give ostree, WebKit, etc several releases in which to change their code to do their own rate-limiting before it became a problem, while gradually removing the unnecessary delays in the case of evolution and other programs that didn't actually need the rate limiting.

*** Bug 723612 has been marked as a duplicate of this bug. ***

Created attachment 299203 [details] [review]
gtask: remove hardcoded GTask thread-pool size

GTask used a 10-thread thread pool for g_task_run_in_thread() /
g_task_run_in_thread_sync(), but this ran into problems when task
threads blocked waiting for another g_task_run_in_thread_sync()
operation to complete. Previously there was a workaround for this, by
bumping up the thread limit when that case was detected, but deadlocks
could still happen if there were non-GTask threads involved. (Eg, task
A sends a message to thread X and waits for a response, but thread X
needs to complete task B in a thread before returning the response to
task A.)

So, allow GTask's thread pool to be expanded dynamically, by watching
it from the glib worker thread, and growing it (at an
exponentially-decreasing rate) if too much time passes without any
tasks completing. This should solve the deadlocking problems without
causing sudden breakage in apps that assume they can queue huge
numbers of tasks at once without consequences.

An alternate approach; I think this way (queue a timeout in another thread, and have it increase the pool size if the timeout is reached) results in simpler code than Olivier's version. It's currently all inside GTask, but it could be moved into GThreadPool in the same way...

I don't think we want this for 2.44.0, but maybe 2.44.1 if it gets some testing?

Nice approach. Do we still want to keep the blocking_other_task logic ? With this approach, will the threadpool size ever come down again, after ballooning ?

(In reply to Matthias Clasen from comment #59)
> Nice approach. Do we still want to keep the blocking_other_task logic ?

Maybe it would be nice to keep it as an optimization. As long as it doesn't complicate the rest of the logic too much; it's possible that having there be two different reasons for max-threads overflow would mess things up. I'd have to try implementing it to figure that out.

> With this approach, will the threadpool size ever come down again, after
> ballooning ?

Yes. When the pool is oversized, max_threads is decreased by 1 every time a task completes. (Which is nearly equivalent to: when the pool is oversized, new threads are only created to run a single task and then they exit.)

Pushed, without bringing back the blocking-other-task logic, since it did make things confusing. I don't think it should be a problem; the first few extra tasks get run pretty quickly, and I doubt there are situations where you have a chain of 50 run-in-thread tasks all blocking each other. But if it becomes a problem I can change it.

*** Bug 756170 has been marked as a duplicate of this bug. ***

(In reply to Dan Winship from comment #61)
> I doubt there are situations where you have a chain of 50 run-in-thread
> tasks all blocking each other. But if it becomes a problem I can change it.

I'm reopening this, due to bug #756170, which has a backtrace with 20 GTask threads and being stuck. There are used:
   glib2-2.46.0-1.fc23.i686
   evolution-data-server-3.18.0-1.fc23.x86_64
   evolution-3.18.0-1.fc23.x86_64

See bug #756170 comment #4 for a little analyze of the backtrace.

re-fixed: now tasks that block other tasks get sorted to the front of
the queue so that finishing existing operations gets prioritized over
starting new ones.

Assuming no problems show up remind me to cherry-pick this to the
stable branch for .2.

For a reference, it's this commit:
https://git.gnome.org/browse/glib/commit/?id=4dae2d8289afabb59e3889118c392a09

I gave it a try by applying the commit on top of glib 2.46.0 release and with it I'm able to run my Evolution, while without it I just get stuck GTask tasks with no progress.

Thus please do apply it to the stable branch as well. Thanks in advance.

(In reply to Milan Crha from comment #65)
> Thus please do apply it to the stable branch as well. Thanks in advance.

done

The section "Resource Management" in http://joeduffyblog.com/2015/11/19/asynchronous-everything/ is pretty interesting related to this.

Given https://bugzilla.gnome.org/show_bug.cgi?id=687223#c55 I'm not sure I'd say this bug is fixed, as we're just leaving things up to the application, and I think we could do better.  On the other hand it doesn't really matter that this is technically marked fixed, so I'll just leave it.

Update: Some new proposed patches for the Linux kernel to move threadpooling/async for lots of syscalls into the kernel: 

https://lwn.net/Articles/671649/