All fz_contexts share one lock set, so rendering does not scale across threads

[nampakse30-afk]

Summary

Every fz_context in a process shares one set of mutexes, so MuPDF work does not run in parallel across threads. On a 6-core machine, ten threads each reading the text of a 160-page PDF take 13.3x the single-thread time. Ten processes doing the same work take 3.3x, so the difference is not the hardware.

Cause

wrapper/context.c declares the mutex array at file scope and the callbacks ignore the user pointer MuPDF passes them:

static pthread_mutex_t mutexes[FZ_LOCK_MAX];       // line 8, one set for the whole process

static void lock(void *user, int lock)
{
    (void)user;                                    // line 14
    (void)pthread_mutex_lock(&mutexes[lock]);
}

static const fz_locks_context locks = { NULL, lock, unlock };   // line 33, user = NULL

Every context built by mupdf_new_base_context() therefore locks the same three mutexes, whatever else it has of its own. MuPDF takes FZ_LOCK_ALLOC around every allocation and free, so rendering from several threads spends its time queueing there.

MuPDF already supports one lock set per context: fz_new_context_imp copies fz_locks_context by value, user pointer included, and hands that pointer back to every callback. Nothing is being worked around here — the user pointer is simply unused.

Measurements

Six physical cores (i7-8700K), each thread opening its own file and reading the text of all 160 pages:

threads	before	after
1	1.00x	1.00x
5	5.33x	1.07x
10	13.3x	2.01x
20	29.7x	3.61x

2.01x at ten threads is the machine, not a bottleneck: five threads fit in the physical cores and scale perfectly.

Both halves are required. Allocating a lock set per context changes nothing on its own, because Context::get in mupdf reaches new threads through fz_clone_context, and a clone inherits its parent's lock set. Giving each thread an independent base context without fixing the callbacks changes nothing either — we measured 12.9x that way, indistinguishable from the original.

Two further problems in the same function

Found while preparing a fix, both independent of the above:

1. Use after free. If fz_register_document_handlers throws, fz_catch drops the context and then execution falls through to fz_set_warning_callback(ctx, ...) on it (lines 76-80). Needs a return NULL.

2. Failure path destroys mutexes other contexts are using. When fz_new_context returns NULL, mupdf_new_base_context calls mupdf_drop_base_context(ctx) with ctx == NULL (lines 68-72), which runs pthread_mutex_destroy over the shared array while other live contexts may still hold it.

The current cleanup is also unsound in general: mupdf_drop_base_context destroys the process-wide mutexes whenever any base context goes away.

Fix

We have been running a patch in production for a while:

• allocate the mutex array in mupdf_new_base_context and pass it as locks.user; have the callbacks use it
• mupdf_drop_base_context reads ctx->locks.user and only tears the lock set down when this is the last context in its family (ctx->master == ctx && ctx->context_count == 1), which makes it safe to call on a clone
• mupdf gives each thread an independent base context rather than a clone, which is what makes the per-context lock set reachable

That last one is a trade rather than a pure win — each thread then carries its own store and glyph cache, so memory per thread is higher — so it probably wants to be a choice rather than a new default.

Happy to open a PR for any part of this, or all of it, if you'd like it in a particular shape. fz_context is a complete type to the wrapper (internal.h includes mupdf/fitz.h), so reading ctx->locks.user back needs no new API.

Measured against mupdf 0.8.0 / mupdf-sys 0.8.0 on Linux.

[messense]

Pull requests are welcome!