Skip to content
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.

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

Make Redcarpet::Markdown#render thread safe #672

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tessereth
Copy link

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.

ClearlyClaire added a commit to ClearlyClaire/mastodon that referenced this pull request May 23, 2019
ClearlyClaire added a commit to glitch-soc/mastodon that referenced this pull request May 23, 2019
rtucker pushed a commit to vulpineclub/mastodon that referenced this pull request Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant