Conversation
clokep
left a comment
clokep
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Seems like it would be a nice improvement.
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
|
Looks like this needs |
|
Yes, I ran black and fixed the formatting. |
clokep
left a comment
clokep
left a comment
There was a problem hiding this comment.
I think this looks OK, but the logic is a bit confusing. I need to dig a bit into some edge cases and see if it can cause any problems.
| # If the name matches, you're all set and we found the block! | ||
| if node.name == block_name: | ||
| return node.render(context) | ||
| return node, context.render_context |
There was a problem hiding this comment.
I'm having some trouble wrapping my brain around some of this change, it seems like _find_template_block_nodelist really only cares about the render_context after this change and not the context itself.
(This is also true of _find_template_block.)
There was a problem hiding this comment.
The Context/RequestContext has the data to be rendered. The context.render_context has only the template structure that can be reused, so I always need a new Context, but can reuse the render context.
| return _render_template_block(template, block_name, context_instance) | ||
| except BlockNotFound: | ||
| # The block wasn't found in the current template. | ||
| node, render_context = _NODES_CACHE[cache_key] |
There was a problem hiding this comment.
I'm slightly worried that a global cache might not be safe to use here, but it likely is OK in almost every case.
There was a problem hiding this comment.
I agree. Django has a cache per instance of template cached loader. https://github.com/django/django/blob/21d8ea4eb3e22d458515b5278c8cd3a15b069799/django/template/loaders/cached.py#L16
Django template loaders have to handle multiple loader instances and do things like template overriding, that are impossible with a global cache.
This lib is a lot simpler than Django template loaders, so the global cache would work fine.
| if not template.engine.debug: | ||
| _NODES_CACHE[cache_key] = node, render_context | ||
|
|
||
| context_instance.render_context = render_context |
There was a problem hiding this comment.
There's a comment added in #25 that implies reusing render contexts might be unsafe. That does have a test though so hopefully it is OK.
There was a problem hiding this comment.
I think it is ok.
The issue #25 fixed Context object reuse, but we're just reusing the render_context inside a new Context. https://github.com/django/django/blob/main/django/template/loaders/cached.py#L72
|
I added a commit here to support anonymous templates. It is a case where |
3 participants
It is really slow to parse templates everytime you call
render_blockand it is a static information, so you can cache it in memory.I did some benchmarking here and this node cache improved performance ~20%.
Most part of the work was done by @walison17, I just wrote a test and did the benckmark.