redcarpet icon indicating copy to clipboard operation
redcarpet copied to clipboard

Make Redcarpet::Markdown#render thread safe

Open tessereth opened this issue 6 years ago • 0 comments

I ran into the same issue as #570 and I've been trying to think of a good way to fix the issue. The solution is not entirely obvious but as far as I can tell, there are a few options:

  1. Update the documentation to say that Redcarpet::Markdown is not thread safe and you should make one per thread.
  2. Add a global rendering lock. That's what I have here. Based on benchmarks on my laptop, this has an ~2% performance hit for the single threaded case and you get no benefit from concurrency but it doesn't just crash the program.
  3. Split the state into the fixed state (that can be shared between threads) and the rendering state (that needs to be separate). This was the suggestion in that issue. I made a rough attempt at that but sd_markdown->work_bufs is used to cache memory buffers between renders. This doesn't work in the concurrent world and if we just re-create them for every render there's a noticeable performance hit on subsequent renders using the same Redcarpet::Markdown object (~7%). Plus the active encoding is stored in the renderer options making that also not thread safe so we'd have to split those options in two as well.

While I could probably implement option 3, I'm not sure it's possible without at least a minor performance hit. So my questions for you are

  1. Is this a thing you want fixed?
  2. Is this a thing you want fixed enough to warrant a performance hit? If so how much?
  3. Do you think there's value in actually making render concurrent? It's fairly likely this will speed up the concurrent case but slow down the sequential case.

tessereth avatar Jan 27 '19 04:01 tessereth