Issue 3750 - Avoid re-generating subscription list for each locale

Issue 3750 - Avoid re-generating subscription list for each locale

[kzar, 2016-03-08 15:28:33]

Patch Set 1

[Sebastian Noack, 2016-03-08 15:54:32]

Note that jinja2 would already automatically cache that filter, if we wouldn't
parse the template again for each locale, which is yet another reason why page
generation is slow. I know, as I looked into it before, that with the current
architecture it wouldn't be too easy to reuse parsed templates, but perhaps it's
still worth to look into it again.

[Wladimir Palant, 2016-03-09 11:12:28]

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
File filters/get_subscriptions.py (right):

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
filters/get_subscriptions.py:24: def get_subscriptions(_):
This shouldn't be a filter but rather a global function, then we wouldn't need
this bogus parameter - I think we didn't have globals back when we implemented
this. Could you move this file into globals/ and change the call while we are at
it?

[Sebastian Noack, 2016-03-09 11:18:17]

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
File filters/get_subscriptions.py (right):

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
filters/get_subscriptions.py:24: def get_subscriptions(_):
On 2016/03/09 11:12:27, Wladimir Palant wrote:
> This shouldn't be a filter but rather a global function, then we wouldn't need
> this bogus parameter - I think we didn't have globals back when we implemented
> this. Could you move this file into globals/ and change the call while we are
at
> it?

I agree that this should rather be a global. However, note that caching
templates, as I suggested before, won't automatically cache that function
anymore if it's a global rather than a filter.

We might want to consider though, also caching templates (by a shared
environment), rather than creating a new environment and parse each template for
every page and locale.

[Wladimir Palant, 2016-03-09 11:25:52]

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
File filters/get_subscriptions.py (right):

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
filters/get_subscriptions.py:24: def get_subscriptions(_):
On 2016/03/09 11:18:17, Sebastian Noack wrote:
> I agree that this should rather be a global. However, note that caching
> templates, as I suggested before, won't automatically cache that function
> anymore if it's a global rather than a filter.

That's fine IMHO, with something like this function it cannot really be decided
automatically that memoization is safe - which is why we should do it
explicitly.

> We might want to consider though, also caching templates (by a shared
> environment), rather than creating a new environment and parse each template
for
> every page and locale.

That's something we can look into, I didn't really consider it a priority as
long as the performance is still acceptable - IMHO independent of the issue at
hand.

[Sebastian Noack, 2016-03-09 11:54:18]

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
File filters/get_subscriptions.py (right):

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
filters/get_subscriptions.py:24: def get_subscriptions(_):
On 2016/03/09 11:25:51, Wladimir Palant wrote:
> That's fine IMHO, with something like this function it cannot really be
decided
> automatically that memoization is safe - which is why we should do it
> explicitly.

Whether you think it's safe or not, jinja's optimizer computes filters and
inlines the result at compile time wherever possible, so whenever the input is
just a constant value and the output can represented as Python literal. However,
since the template is currently parsed again every time, also this filter is
called again. But that optimization doesn't work anyway for globals, as jinja
doesn't know at compile time whether the called function is an environment's
global or whether it's given/overridden by the context at runtime. But I agree,
it's still better to use a global here.

> > We might want to consider though, also caching templates (by a shared
> > environment), rather than creating a new environment and parse each template
> for
> > every page and locale.
> 
> That's something we can look into, I didn't really consider it a priority as
> long as the performance is still acceptable - IMHO independent of the issue at
> hand.

Yes, if we make it a global anyway, it's unrelated.

[Wladimir Palant, 2016-03-09 13:50:15]

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
File filters/get_subscriptions.py (right):

https://codereview.adblockplus.org/29337941/diff/29337942/filters/get_subscri...
filters/get_subscriptions.py:24: def get_subscriptions(_):
On 2016/03/09 11:54:18, Sebastian Noack wrote:
> Whether you think it's safe or not, jinja's optimizer computes filters and
> inlines the result at compile time wherever possible, so whenever the input is
> just a constant value and the output can represented as Python literal.

This makes sense because for filters same input should always produce the same
output. Which again shows that what we have here is *not* a filter.

[kzar, 2016-03-09 15:56:09]

Patch Set 2 : Turned get_subscriptions into a global function

[Sebastian Noack, 2016-03-09 16:09:58]

LGTM

[Wladimir Palant, 2016-03-09 16:14:47]

LGTM