django-phased
django-phased copied to clipboard
codysoyland
Unpickle context only from `phased` blocks
Function utils.second_pass_render tried to unpickle stashed context from all bits of content resulting from splitting of template by PHASED_SECRET_DELIMITER.
But only odd bits are results of phased block rendering, so only those bits contain stashed context.
Even bits can't contain stashed context and since they are results of first pass rendering they can be arbitrary long and even for content of sane length, searching for regular expression match done in utils.unpickle_context function can take insanely long time (which is the source of problems reported in #9).
Fixed by unpickling context only from odd bits.
Fixes: #9
This pull request does not change output of any function and utils.second_pass_render is already covered in existing tests.
It only affects performance for cases described in #9 . So do you mean some benchmarking code, or some form of relative performance test?
@jkuczm Something that explains what you mean with "But only odd bits are results of phased block rendering, so only those bits contain stashed context."
In other words, if the tests pass currently it means we have either broken tests or too few tests.
As I understand the code:
In first pass rendering everything outside of {% phased %}...{% endphased %} blocks is rendered as usual. What's inside those blocks has appended pickled version of wanted variables from context and is surrounded by PHASED_SECRET_DELIMITER.
In second pass rendering template is split by PHASED_SECRET_DELIMITER into series of bits. Those bits are enumerated and ones with even indices (starting from 0) are already rendered parts of template that were outside of phased blocks, so they don't contain pickled context. Bits with indices being odd numbers are content of phased blocks with appended pickled context.
Current version of code in second pass rendering calls unpickle_context on both: bits with even indices and bits with odd indices. Calling unpickle_context on bits with even indices does not break anything, it's just unnecessary, since we know there can't be any pickled context and unpickle_context will return None. This unnecessary thing can take very long time in some circumstances.
It's like adding time.sleep to body of any function, unit tests won't fail because of this.
So I don't think that fact that tests pass for current version indicates any gap in them.
This should be merged in. I think it is completly correct and It leads to better performance (in my case page is retrieved two times faster).
I was noticing this problem as well, and I can't think of a way to test this change without breaking the function apart and making the testing dependent on the current implementation. There doesn't appear to be any performance tests in the code right now, so this fix doesn't change the status quo.
I did add a couple of comments which hopefully make the code more clear: https://github.com/codysoyland/django-phased/pull/16
