djangobars icon indicating copy to clipboard operation
djangobars copied to clipboard

Caching and thread safety (largeish)

Open schuyler1d opened this issue 9 years ago • 1 comments

This is a large-ish patch that adds thread locks where necessary and precompilation support. I think it's reasonable that we don't necessarily want to accept this patch, but I wanted to get feedback on whether this is something you'd be willing to accept, either in a more completed form, or after we get all the way to certain goals.

This is meant to gesture towards the following goals:

  • maintaining backwards compatibility
  • moving towards support for Django 1.8
  • allowing djangobars to be used in a multi-threaded environment

This work came about when originally trying to deploy djangobars in a multithreaded production environment with high traffic. Two kinds of errors surfaced that lead to a (temporary) rollback. The first were context errors -- logic like {{#if foo.bar}} {{foo.bar}} {{/if}} would fail saying 'no foo.bar' exists! The second is that templates were being outputted in corupted ways. It became clear that there was a threading issue, and sure-enough, in pybars._compiler, there's a note saying that the Compiler is not thread-safe.

After fixing thread-safety, there was still a big issue with performance. Locking the threads destroyed the performance value of a threaded environment. Measuring performance, the bottleneck was compiling the templates. Thus, the precompilation support.

It is organized as follows:

  • In anticipation of Django 1.8 template engine support, it creates a new file template/backends.py with a Handlebars engine structured for the Django 1.8 api (though missing some parts for full support)
  • In backends.py there is also a CompiledTemplate which is subclassed from template.base.HandlebarsTemplate
  • HandlbarsTemplate and BaseHandlebarsLoader are tweaked to leverage the code in backends.Handlebars.
  • loader.reset_loaders is a new method, which helps testing
  • loaders.filesystem.CachedLoader is mostly separate, but also helps keep template compilations in cache and reusable for future requests.

Some current issues:

  • Although we get pretty far to Django 1.8 support, it's not complete and certainly hasn't been tested. Besides testing, at minimum the backends.Handlebars.get_template needs to be able to handle a real dirs list rather than being passed the legacy loaders objects in its place. When running the test suite against current django 1.8, we clearly also need to try/catch some django imports that no longer exist there.
  • Because of the dual goals of maintaining the old interfaces, along with moving toward Django 1.8, there's a lot of argument passing and not a lot of clarity on which objects are responsible for what. Firstly, feedback is welcome on how to organize it better. Second, once we made a 'true' Django 1.8 engine, it might be better, even if we kept the goal of maintaining backward compatibility that the old interfaces just be re-implemented from the engine, rather than what I'm doing which is more the other way around. Alternatively, we could save that for another day.
  • Currently, there's no testing on whether a template has been updated when using the pre-compiled version. We could leave it that way, and just document, that the user is responsible for clearing the directory when there are updates. We could also do something fancy with file mtimes and possibly addending the 'full_code' that comes back from pybars.

Originally, I separated the thread safety and precompilation as projects, but in production, we found that locking threads, still blocks too many requests, so they are really necessary combined, at least, in practice.

schuyler1d avatar Jun 18 '15 19:06 schuyler1d

Hey @schuyler1d, thanks for this. I'll go through it as soon as I have time -- likely this coming weekend. I'm excited to incorporate the template engine updates from Django 1.8 (e.g., issue #5 ). Since it is such a significant new "feature", I was even considering making a backwards-incompatible branch that supports Django 1.8+.

I'll share more thoughts after I go through your patches. Thanks again!

mjumbewu avatar Jun 22 '15 15:06 mjumbewu