filters/get_subscriptions.py - Issue 29337941: Issue 3750 - Avoid re-generating subscription list for each locale

Delta Between Two Patch Sets: filters/get_subscriptions.py

Issue 29337941: Issue 3750 - Avoid re-generating subscription list for each locale (Closed)

Left Patch Set: Created March 8, 2016, 3:27 p.m.

Right Patch Set: Turned get_subscriptions into a global function Created March 9, 2016, 3:53 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 # This file is part of the Adblock Plus website,

2 # Copyright (C) 2006-2016 Eyeo GmbH

3 #

4 # Adblock Plus is free software: you can redistribute it and/or modify

5 # it under the terms of the GNU General Public License version 3 as

6 # published by the Free Software Foundation.

7 #

8 # Adblock Plus is distributed in the hope that it will be useful,

9 # but WITHOUT ANY WARRANTY; without even the implied warranty of

10 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

11 # GNU General Public License for more details.

12 #

13 # You should have received a copy of the GNU General Public License

14 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

15

16 import codecs

17 import os

18 import tarfile

19 import urllib

20 import logging

21 from ConfigParser import SafeConfigParser

22 subscriptions = None

23

24 def get_subscriptions(_):
Wladimir Palant 2016/03/09 11:12:27 This shouldn't be a filter but rather a global fun 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 I agree that this should rather be a global. Howev Show quoted text 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:51 That's fine IMHO, with something like this functio Show quoted text 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. Show quoted text > 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 Whether you think it's safe or not, jinja's optimi Show quoted text 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. Show quoted text > > 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 This makes sense because for filters same input sh Show quoted text 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.
25 global subscriptions

26 if not subscriptions is None:

27 return subscriptions

28 try:

29 from sitescripts.subscriptions import subscriptionParser

30 except ImportError:

31 logging.warning("Unable to import sitescripts, proceeding with empty "

32 "subscriptions list.")

33 return []

34

35 result = {}

36 utf8_reader = codecs.getreader('utf8')

37 source = urllib.urlopen("https://hg.adblockplus.org/subscriptionlist/archive/d efault.tar.gz")

38 orig_get_settings = subscriptionParser.get_settings

39 try:

40 # Hack: monkey-patch subscriptionParser.get_settings()

41 settings = SafeConfigParser()

42 settings_handle = urllib.urlopen("https://hg.adblockplus.org/subscriptionlis t/rawfile/default/settings")

43 try:

44 settings.readfp(utf8_reader(settings_handle))

45 finally:

46 settings_handle.close()

47 subscriptionParser.get_settings = lambda: settings

48

49 with tarfile.open(fileobj=source, mode="r\|gz") as archive:

50 for fileinfo in archive:

51 if os.path.splitext(fileinfo.name)[1] != ".subscription":

52 continue

53

54 filedata = subscriptionParser.parse_file(fileinfo.name, utf8_reader(arch ive.extractfile(fileinfo)))

55 if filedata.unavailable:

56 continue

57

58 result[filedata.name] = filedata

59 finally:

60 source.close()

61 subscriptionParser.get_settings = orig_get_settings

62

63 subscriptionParser.calculate_supplemented(result)

64 subscriptions = result.values()

65 return subscriptions

LEFT	RIGHT