Providing thread context for custom allocator

[ebraminio]

Support for custom allocators inside harfbuzz were just added on harfbuzz recently and as main clients of harfbuzz use it on single threaded fashion and with no-custom allocators, the following issue probably has not high priority but would be nice if we can improve the situation.

I was interested on the comment that is put here and whether we can improve the situation for them so I asked the commentator for more explanation on an email and took permission to publish it here,

We use Harfbuzz in MuPDF. I'll explain the biggest problem we have with it.

In common with many projects out there MuPDF has it's own allocation system - so everything should call fz_malloc and fz_free, rather than normal malloc and free.

i.e. we need to make everything allocate through:

void *fz_malloc(fz_context *ctx, size_t size);

This includes Harfbuzz's allocations. Now, Harfbuzz tries to accomodate us - we can set hb_malloc_impl=hb_malloc and hb_free_impl=hb_free etc during compilation, to direct calls to hb_malloc and hb_free functions that we supply, that call down to the MuPDF allocators.

So we define:

 void *hb_malloc(size_t size)
 {
     return fz_malloc( ???? , size);
 }

But what goes ???? here?

If MuPDF were single threaded, then we could have:

static fz_context *hb_ctx;

and then have:

 void *hb_malloc(size_t size)
 {
     return fz_malloc(hb_ctx, size);
 }

and as long as we set hb_ctx before we call into Harfbuzz, we'd be fine.

Unfortunately, for multi-threaded stuff this doesn't work, because the fz_context is (and needs to be) different in every thread.

We can't safely use a single static fz_context pointer in a multi-threaded build.

We could use thread local storage, but that's a problem for us. So far we've managed to avoid relying on that, and it doesn't fit nicely into our use of threading constructs, so we've gone for a different solution.

Currently we simply take and drop a mutex around all calls to harfbuzz.
This means that only 1 thread can be calling into harfbuzz at a time, and we can safely get away with a single static fz_context. It's not a nice solution though.

As another point - It might be nice to work with a system level shared Harfbuzz library, but this can never work, because the system level lib won't redirect hb_malloc and hb_free as we need.

Proposed solution:

Well, having thought about it, there is a simple (but tedious) fix that addresses ALL these issues, preserves the ABI, and is keeping with what the rest of the world does with such libraries.

Suppose we have a harfbuzz API function:

hb_wibble(A, B, C)

Rename that, and add 1 extra param to make it:

hb_wibble_threaded(hb_alloc_ctx *ctx, A, B, C)

then add:

  hb_wibble(A, B, C)
  {
      return hb_wibble_threaded(NULL, A, B, C);
  }

So the overall API is still preserved (at the cost of a tiny stub function), but we've gained another API that we can call that is thread safe.

We then work our way through hb_wibble_threaded arranging for 'ctx' to be passed through all the way down to point at which we call malloc or free, where we do something like:

 if (ctx)
   return ctx->malloc(ctx->opaque, size);
 else
   return malloc(size);

(We can wrap this as malloc_shim or something)

So the idea is that 'ctx' holds the allocator/deallocator functions, and the context for the calling code (ctx->opaque).

Essentially at the end of it, we end up with Harfbuzz being structured exactly as before, but now offering 2 APIs - one threadsafe, one not.

In addition Harfbuzz can now safely be used as a system level library as things can call it via the threadsafe API and still have control of where the underlying allocations go.

The cost in size should be trivial, and the cost in maintainability is very low too - all that we lose is the need to pass this extra context down through the code.

[behdad]

I'm confused. HarfBuzz definitely IS completely threadsafe to the best of my knowledge. That was one of the strong design goals. I have had to maintain, debug, and fix threadsafety issues in FreeType, fontconfig, and Pango over the years, so I definitely know the issues and tried to address them all in HarfBuzz.

• It is utterly thread unsafe. It uses statics to hold
• structures in, meaning that if it is ever called from
• more than one thread at a time, access to such
• structures can race.

This can't be farther from true. I use compare-and-exchange to safely implement singleton initializations or prepend-only linked-lists as caches. How is that not threadsafe?

Runtime-configurable custom allocators are different issue. I don't believe they are a useful feature and have not yet seen a compelling usecase.

[khaledhosny]

MuPDF seems to build HarfBuzz HB_NO_MT defined, so no wonder their version of HarfBuzz is not thread-safe.

[khaledhosny]

Also the code linked above seems to declare many of its internal functions with hb_ prefix, but that is not a good idea as it might clash with future HarfBuzz functions. It should use a unique prefix, may be fz_hb_ or some such.

[robinwatts]

Hi, I am the original author of the comment that about harfbuzz being thread unsafe. If I am wrong, then I would love to be corrected, and I will apologise unreservedly. Having said that, I've just looked again, and I still see stuff that I am concerned about.

I am sorry you feel that "Runtime-configurable custom allocators... [are not a] useful feature."

Within MuPDF we feel they are crucial to our behaviour, and they cut right to the heart of this issue.

MuPDF is at pains to direct all its allocations through fz_malloc/fz_free etc. These in turn are passed down to whatever runtime configurable allocators are passed to MuPDF. Every library used by MuPDF can handle this just fine - it is just Harfbuzz that requires special treatment here. Harfbuzz being different to all the other libraries we use is a clear indication that it's "non-standard", at least.

Why do we do this? Well, so we can get accurate measurements on how much memory is used for one. This is useful, not just for debugging purposes, but also because it enables us to run a scavenging memory manager.

This memory manager enables us to tune our performance to run in restricted amounts of memory. If a memory allocation fails, we can intelligently clear buffers etc on an LRU basis (discarding decoded fonts, images, cached information etc) until the allocation succeeds.

This allows us to avoid the traditional tradeoffs between "cache everything and run out of memory" and "cache nothing and run really slowly".

In addition, we run tests with a memory lib that a) lists leaked blocks on exit, and b) performs memory squeezing tests. (Memory squeezing effectively runs the program multiple times failing the allocations at all the different points, thus ensuring we exit neatly in all cases.) The presence of hb blocks held in statics means we have to have specific hacks in there to allow these tests to run clean.

(I distrust browser based editing, so I'll break the comment here and continue in another one :) )

[robinwatts]

So, the issues I have with harfbuzz are:

1. The use of statics
2. The lack of runtime-specific allocators
3. The lack of a runtime context.

I've talked about why 1 and 2 are a problem above. Now let me talk about why 3 is a problem for us.

Throughout MuPDF we use a setjmp/longjmp mechanism for nested error handling (wrapped up into an set of fz_try/fz_always/fz_catch macros that should be familiar to anyone who has used C++ or Java).

Because this is nested, we need a stack to handle the exceptions on. We hold this (together with other 'globals' such as the allocators in use) as an fz_context structure. All our MuPDF calls take an fz_context as the first argument.

Because we can be running in multiple threads at the same time, we have multiple different nestings going on of errors, hence we need multiple different error stacks, hence each thread has its own fz_context.

We make use of these fz_throw/fz_catch calls within our own allocator functions (to implement the scavenging I referred to earlier). Hence when function A calls into harfbuzz, and it calls back to (say) fz_malloc, we need to somehow obtain the context that A had within fz_malloc.

With most other libraries, they offer the ability to pass in an opaque "void *" which is passed through to allocator calls. Harfbuzz does not offer this facility.

We are having to resort to storing it in a static variable, and then using a mutex to protect against multi-threading. Not ideal.

(more in the next comment)

[robinwatts]

So, how are we working around all this in MuPDF currently?

Well, we build harfbuzz statically with:

hb_malloc_impl=hb_malloc
hb_free_impl=hb_free
hb_realloc_impl=hb_realloc

(Yes we can, and probably should rename these to be fz_hb_... instead)

We also serialise calls to Harfbuzz so only one call at a time is ever made (hurting our performance).

This means that MuPDF gets a single threaded access to your excellent and invaluable lib that's reasonably safe, at the cost of some reduced performance.

Why only reasonably safe? Well, we're back to 1) and 2).

Consider the following scenario.

libMuPDF is built relying on libHarfbuzz

libA is built relying on libMuPDF
libB is built relying on libMuPDF.

Now, someone comes along and writes an app, Z, that uses both libA and libB.

Z calls libA. libA sets up the memory allocators, and calls libMuPDF, which calls libHB, which causes your tables of statics to be filled out (using the memory allocators supplied by libA). Then libmuPDF exits, libA exits, and control returns to Z.

Now Z calls libB. libB sets up its own memory allocators, and calls libMuPDF, which calls libHB. This makes use of the static tables that were set up as part of libA's operation.

But how do we know that these are safe any more? For all we know the memory that this relies on may have been trashed when libA closed its allocators down.

You may claim that this is far-fetched, but such problems could be completely sidestepped by simply working in the standard way that all other libraries use.

Now, of course, Harfbuzz is your baby, and it's a really great, useful library that you are providing to the community free of charge. I do not wish to be ungrateful, or appear to be stamping my feet in an entitled manner saying "YOU MUST FIX THIS!"

I mention the flaws (as I perceive them) in the library merely as information. If I can persuade you to rejig the library slightly to make it more-standard and even more useful than it already is, then great. If you are not interested in amending the lib, then so be it.

If there is some magic build setting that I can use to alleviate some or all of these problems, please tell me. I will happily be made to look foolish if it will lift some of the problems :)

Thanks for listening.

[behdad]

Robin,

I have no personal attachment to HarfBuzz anymore. And I don't take anything you write personally. However, looks like you have either not read my comment and Khaled's comment, or have not understood them. I understand all issues that you talk about, but there's no point in me replying in detail while you refuse to understand the most basic part of our points. Let me reiterate:

• HarfBuzz is fully threadsafe. The use of static variables is threadsafe as well. If you don't understand how, then you need to learn more about implementing threadsafe structures,

• The only time HarfBuzz is NOT threadsafe, is when you build it with HB_NO_MT set, which as Khaled pointed out, seems to be what mupdf is doing.

So, basically, you built a nonthreadsafe HarfBuzz, then are blaming the project for being "utterly thread unsafe".

[robinwatts]

Behdad,

I have read your comment, and Khaled's. I like to think I understood them.

Allow me to rephrase my objections. I should not have said "Harfbuzz is utterly thread unsafe". My apologies.

Harfbuzz is indeed "fully threadsafe" if you stick with just malloc and free as the allocators.

For us, however, this is not an option. The moment you allow Harfbuzz to be built so that allocations can be redirected to other routines, you are placing (IMHO) unreasonable restrictions on the way those allocators have to behave (w.r.t to their lack of any context, and the lifespan of the returned memory).

What I should have said therefore is something like: "Harfbuzz is impossible to use in a multithreaded environment without placing (IMHO) unreasonable requirements on the behaviour of the underlying allocators".

Your definition of what constitutes "unreasonable requirements" may differ from mine, of course :)

And, yes, we build with HB_NO_MT, because a) not using HB_NO_MT would not help us overcome the problems with the allocators, and b) the workarounds we are forced to do in our calling code to sidestep the allocator limitations means that we no benefit(*) from building with HB_NO_MT. Thus we avoid relying on the platform specifics in that section of the code.

(* Or if we get a benefit, I haven't spotted it in practice).

To put this another way: If I built without HB_NO_MT, would it solve a single one of the problems I described above?

[robinwatts]

Actually, let me go further.

I apologise unreservedly for the comments quoted above, namely that:

It is utterly thread unsafe. It uses statics to hold
structures in, meaning that if it is ever called from
more than one thread at a time, access to such
structures can race.

I'll work on getting those removed from the MuPDF source code, and replaced with a better statement of why we need the workarounds we do.

Sorry.

[ebraminio]

No problem Robin, I believe what you wanted is not out of scope for the project, maybe this can not be considered now for some reasons (not willing to make things complicated or ABI/API breakage or anything) but can/may be considered for the next iteration of this software or any other libraries design. Thanks for your detailed and informative comments and your participation here 😊

[behdad]

Robin,

You have indeed summarized very well why using custom allocators is a bad idea. I feel your pain but have no empathy for you. If I was you, I'd remove them. They don't solve a real problem IMO. It's fine to agree to disagree :).