Zola: Make a SyntaxSet pool

[Keats]

See https://github.com/trishume/syntect/issues/20 for the syntect issue

Either do a pool in gutenberg or in Syntect as mentioned in the issue above. Doing it in syntect would be nicer as any syntect users will get it instead of duplicating the work in every syntect users but I'm not against having it only in gutenberg.
This currently prevents the markdown rendering from being made parallel but I'm not entirely sure what's the best way to make it work.

Opening this issue in case someone wants to work on that, that would be super helpful!
The code for SyntaxSet should be in rendering/highlighting.rs

[williamyaoh]

Wrote up a quick and dirty implementation of parallel markdown rendering; it's just using thread_local!() and a thread pool to initialize a single SyntaxSet per thread.

Right now it's just creating a separate thread pool per call to render_section(), but the thread pool
initialization should probably be done once for the entire program. Otherwise, each section would pay the price for initializing the SyntaxSets. However, I'm not sure of the best place in the code to wrap the whole site/program in a thread pool.

[williamyaoh]

The scoped thread pool I wrote up is also slightly dubious, but I think I can rewrite it in terms of crossbeam::scope() and not use any unsafe code.

I think that building the whole site should still fail if any of the pages/sections fail to render, but I'm also unsure of how best to propagate rendering errors from the worker threads to the main thread, even if rewriting the thread pool to use crossbeam.

[williamyaoh]

Rewriting syntect to make SyntaxSet Send + Sync would still be the best solution, so that only a single SyntaxSet has to be initialized per program run, but that's a lot more effort than creating a thread pool in gutenberg. I can take a look at rewriting syntect, though; I suspect that something like this solution would only be a stopgap anyways.

[Keats]

I'm not sure it would be possible to have SyntaxSet be Send + Sync from the issue linked in the first post but that would definitely be the best solution as other users of syntect would also benefit from it. That's a big chunk of work though afaik

[Keats]

@williamyaoh any update on that?

[williamyaoh]

@Keats After taking a look at syntect and asking trishume, it looks like adding some way to let syntect users decide whether SyntaxSet used thread-safe or non-thread-safe interior mutability (and thus choose whether SyntaxSet is Sync or not) wouldn't be feasible, since syntect is relying on RefCell passing Eq and PartialOrd implementations on the contained data through, but RwLock isn't transparent for those traits. As trishume also mentioned in the other issue, he probably wouldn't accept such a solution even if it did work, due to the synchronization overhead.

Right now I'm running with your idea of the ParallelSyntaxSet; I've got a working version which does the pooling directly in syntect, while leaving the actual threading up to library users, but I still need to write some tests and benchmarks to make sure it's working properly. I'll be posting an update in the syntect issue when I'm more confident that it works the way it should.

[williamyaoh]

Back to the issue of moving thread pooling up to the level of rendering the whole site, instead of just at the level of rendering individual sections:

With the prototype I have right now, a thread pool is being created each time a section is rendered. This works to parallelize rendering/highlighting, but has some issues:

1. Each new thread is initializing a SyntaxSet; this is okay for sections with lots of pages in them, but is otherwise detrimental.
2. It's spawning more threads than necessary. We only need at most $NUM_CPUS threads for the entire site, but I'm creating a lot more than that. There's no need to pay the price of allocating and deallocating stack space on each thread spawn/join when we know how many threads we'll need ahead of time.

I've opened a pull request on syntect with my pooling solution which would solve (1); however, (2) is still a problem. We can't really use existing solutions; the rendering in render_section has to use function-local data, so we'd need something like crossbeam::scope, but we also need thread pooling, so neither thread_pool nor crossbeam solves the problem by themselves, and we can't use them in combination. I can think of two solutions:

1. In the method to render a site (I can't remember what it's called), do a threading::scoped_pool() there instead of inside render_section; all the inner code to render the site gets moved into the closure, and all functions on the call path to render_section get an extra parameter for the scoped thread pool.

   This has the advantage of not requiring any unsafe code, but requires an extra parameter on a bunch of functions which they won't use at all besides passing it down.

2. Rewrite the thread pool to be static. Something like this:

   lazy_static! {
      pub THREAD_POOL: ThreadPool = ThreadPool::with_pool_size(8);
   }
   
   THREAD_POOL::scope(|scope| {
      scope.spawn(move || {
          // do stuff in parallel here
      });
   });
   

   That way, the parallel code could be limited to just render_section, where it's actually necessary. This has the advantage of requiring the minimum amount of code to be changed inside gutenberg, but will require unsafe blocks to write.

Personally, I'm in favour of the second approach, so as not to disturb the existing code and leave it easier to maintain.

[Keats]

My PoC before syntect was thread safe (nice job!) was using https://github.com/nikomatsakis/rayon on https://github.com/Keats/gutenberg/blob/master/src/site.rs#L122-L129, which is the slowest part if a section is using syntax highlighting, I didn't try parallelising render_section at all.

I think the first need would be to write benchmarks so we can see which bits benefit the most from parallelization and then we can try. In your options, I think I prefer 2 but it should be easy to experiment

[williamyaoh]

Right; those are actually the only lines I changed in that commit linked... somewhere up above.

I'll work on option 2 above and adapt those lines to use that approach. We can then use that as a baseline for performance numbers against other approaches.

[Keats]

(Moving the benching discussion to #78)

[williamyaoh]

As it turns out, this was a lot easier than I thought it was, by using rayon. I'll open a pull request with my changes.