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

Caching and thread safety (largeish) #13

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

Conversation

schuyler1d
Copy link
Contributor

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.

…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.

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

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.
@mjumbewu
Copy link
Owner

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!

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.

2 participants