From 907ce30e16ee2a64bfb7d144d144de082ff9cd89 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Mon, 5 Sep 2016 11:58:14 +0200 Subject: [PATCH 01/18] Initial support of catalog caching with plone.memoize --- setup.py | 1 + src/Products/PluginIndexes/interfaces.py | 7 ++ src/Products/ZCatalog/Catalog.py | 40 +++++--- src/Products/ZCatalog/cache.py | 116 +++++++++++++++++++++++ 4 files changed, 149 insertions(+), 15 deletions(-) create mode 100644 src/Products/ZCatalog/cache.py diff --git a/setup.py b/setup.py index 36b938e0..c2b1827f 100644 --- a/setup.py +++ b/setup.py @@ -69,6 +69,7 @@ 'zope.interface', 'zope.schema', 'zope.testing', + 'plone.memoize', ], include_package_data=True, zip_safe=False, diff --git a/src/Products/PluginIndexes/interfaces.py b/src/Products/PluginIndexes/interfaces.py index e1aeb879..27dd9576 100644 --- a/src/Products/PluginIndexes/interfaces.py +++ b/src/Products/PluginIndexes/interfaces.py @@ -283,3 +283,10 @@ def make_query(query): def getIndexNames(): """ returns index names that are optimized by index """ + + +class IIndexCounter(Interface): + """ Invalidation helper API for pluggable indexes""" + + def getCounter(): + """Return a counter which is increased on index changes""" diff --git a/src/Products/ZCatalog/Catalog.py b/src/Products/ZCatalog/Catalog.py index 71ef33f1..77a2f7ae 100644 --- a/src/Products/ZCatalog/Catalog.py +++ b/src/Products/ZCatalog/Catalog.py @@ -29,6 +29,7 @@ from Missing import MV from Persistence import Persistent from ZTUtils.Lazy import LazyMap, LazyCat, LazyValues +from plone.memoize import ram from Products.PluginIndexes.interfaces import ( ILimitedResultIndex, @@ -38,6 +39,7 @@ from Products.PluginIndexes.util import safe_callable from Products.ZCatalog.CatalogBrains import AbstractCatalogBrain, NoBrainer from Products.ZCatalog.plan import CatalogPlan +from Products.ZCatalog.cache import _apply_query_plan_cachekey from Products.ZCatalog.ProgressHandler import ZLogHandler from Products.ZCatalog.query import IndexQuery @@ -594,6 +596,28 @@ def _search_index(self, cr, index_id, query, rs): return rs + @ram.cache(_apply_query_plan_cachekey) + def _apply_query_plan(self, cr, query): + + plan = cr.plan() + if not plan: + plan = self._sorted_search_indexes(query) + + rs = None # result set + for index_id in plan: + # The actual core loop over all indices. + if index_id not in self.indexes: + # We can have bogus keys or the plan can contain + # index names that have been removed in the + # meantime. + continue + + rs = self._search_index(cr, index_id, query, rs) + if not rs: + break + + return rs + def search(self, query, sort_index=None, reverse=False, limit=None, merge=True): """Iterate through the indexes, applying the query to each one. If @@ -619,21 +643,7 @@ def search(self, query, cr = self.getCatalogPlan(query) cr.start() - plan = cr.plan() - if not plan: - plan = self._sorted_search_indexes(query) - - rs = None # result set - for index_id in plan: - # The actual core loop over all indices. - if index_id not in self.indexes: - # We can have bogus keys or the plan can contain index names - # that have been removed in the meantime. - continue - - rs = self._search_index(cr, index_id, query, rs) - if not rs: - break + rs = self._apply_query_plan(cr, query) if not rs: # None of the indexes found anything to do with the query. diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py new file mode 100644 index 00000000..6f1b5787 --- /dev/null +++ b/src/Products/ZCatalog/cache.py @@ -0,0 +1,116 @@ +############################################################################## +# +# Copyright (c) 2010 Zope Foundation and Contributors. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE +# +############################################################################## + +from Acquisition import aq_base +from Acquisition import aq_parent + +from DateTime import DateTime +from plone.memoize.volatile import DontCache +from Products.PluginIndexes.interfaces import IIndexCounter +from Products.PluginIndexes.interfaces import IDateRangeIndex +from Products.PluginIndexes.interfaces import IDateIndex + + +class CatalogCacheKey(object): + def __init__(self, catalog, query=None): + + self.catalog = catalog + self.cid = self.get_id() + self.query = query + self.key = self.make_key(query) + + def get_id(self): + parent = aq_parent(self.catalog) + path = getattr(aq_base(parent), 'getPhysicalPath', None) + if path is None: + path = ('', 'NonPersistentCatalog') + else: + path = tuple(parent.getPhysicalPath()) + return path + + def make_key(self, query): + + if query is None: + return None + + catalog = self.catalog + + key = [] + for name, value in query.items(): + if name in catalog.indexes: + index = catalog.getIndex(name) + if IIndexCounter.providedBy(index): + counter = index.getCounter() + else: + # cache key invalidation cannot be supported if + # any index of query cannot be tested for changes + return None + else: + # return None if query has a nonexistent index key + return None + + if isinstance(value, dict): + kvl = [] + for k, v in value.items(): + v = self._convert_datum(index, v) + v.append((k, v)) + value = sorted(kvl) + + else: + value = self._convert_datum(index, value) + + key.append((name, counter, value)) + + key = tuple(sorted(key)) + cache_key = '%s-%s' % (self.cid, hash(key)) + return cache_key + + def _convert_datum(self, index, value): + + def convert_datetime(dt): + + if IDateRangeIndex.providedBy(index): + term = index._convertDateTime(dt) + elif IDateIndex.providedBy(index): + term = index._convert(dt) + else: + term = value + + return term + + if isinstance(value, (list, tuple)): + res = [] + for v in value: + if isinstance(v, DateTime): + v = convert_datetime(v) + res.append(v) + res.sort() + + elif isinstance(value, DateTime): + res = convert_datetime(value) + + else: + res = (value,) + + if not isinstance(res, tuple): + res = tuple(res) + + return res + + +# plone memoize cache key +def _apply_query_plan_cachekey(method, catalog, plan, query): + cc = CatalogCacheKey(catalog, query) + if cc.key is None: + raise DontCache + return cc.key From a50a84ee676de89d8a48addb008ff4a01dce48d0 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Mon, 5 Sep 2016 14:54:57 +0200 Subject: [PATCH 02/18] Add getCounter logic to TopicIndex --- .../PluginIndexes/TopicIndex/FilteredSet.py | 9 ++--- .../PluginIndexes/TopicIndex/TopicIndex.py | 35 ++++++++++++++++--- .../PluginIndexes/TopicIndex/tests.py | 20 +++++++++++ 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/Products/PluginIndexes/TopicIndex/FilteredSet.py b/src/Products/PluginIndexes/TopicIndex/FilteredSet.py index 6f1bcffb..be0c67e7 100644 --- a/src/Products/PluginIndexes/TopicIndex/FilteredSet.py +++ b/src/Products/PluginIndexes/TopicIndex/FilteredSet.py @@ -41,10 +41,7 @@ def index_object(self, documentId, obj): raise RuntimeError('index_object not defined') def unindex_object(self, documentId): - try: - self.ids.remove(documentId) - except KeyError: - pass + self.ids.remove(documentId) def getId(self): return self.id @@ -78,12 +75,15 @@ class PythonFilteredSet(FilteredSetBase): meta_type = 'PythonFilteredSet' def index_object(self, documentId, o): + res = 0 try: if RestrictionCapableEval(self.expr).eval({'o': o}): self.ids.insert(documentId) + res = 1 else: try: self.ids.remove(documentId) + res = 1 except KeyError: pass except ConflictError: @@ -91,6 +91,7 @@ def index_object(self, documentId, o): except Exception: LOG.warn('eval() failed Object: %s, expr: %s', o.getId(), self.expr, exc_info=sys.exc_info()) + return res def factory(f_id, f_type, expr): diff --git a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py index 95723510..54c1a431 100644 --- a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py +++ b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py @@ -18,6 +18,7 @@ from BTrees.IIBTree import intersection from BTrees.IIBTree import union from BTrees.OOBTree import OOBTree +from BTrees.Length import Length from OFS.SimpleItem import SimpleItem from Persistence import Persistent from zope.interface import implementer @@ -25,6 +26,7 @@ from Products.PluginIndexes.interfaces import ( IQueryIndex, ITopicIndex, + IIndexCounter, ) from Products.PluginIndexes.TopicIndex.FilteredSet import factory from Products.ZCatalog.query import IndexQuery @@ -33,7 +35,7 @@ LOG = getLogger('Zope.TopicIndex') -@implementer(ITopicIndex, IQueryIndex) +@implementer(ITopicIndex, IQueryIndex, IIndexCounter) class TopicIndex(Persistent, SimpleItem): """A TopicIndex maintains a set of FilteredSet objects. @@ -44,6 +46,7 @@ class TopicIndex(Persistent, SimpleItem): meta_type = 'TopicIndex' zmi_icon = 'fas fa-info-circle' query_options = ('query', 'operator') + _counter = None manage_options = ( {'label': 'FilteredSets', 'action': 'manage_main'}, @@ -52,6 +55,7 @@ class TopicIndex(Persistent, SimpleItem): def __init__(self, id, caller=None): self.id = id self.filteredSets = OOBTree() + self._counter = Length() self.operators = ('or', 'and') self.defaultOperator = 'or' @@ -61,22 +65,45 @@ def getId(self): def clear(self): for fs in self.filteredSets.values(): fs.clear() + self._increment_counter() def index_object(self, docid, obj, threshold=100): """ hook for (Z)Catalog """ + res = 0 for fid, filteredSet in self.filteredSets.items(): - filteredSet.index_object(docid, obj) - return 1 + res += filteredSet.index_object(docid, obj) + + if res > 0: + self._increment_counter() + return 1 + + return 0 def unindex_object(self, docid): """ hook for (Z)Catalog """ + res = 0 for fs in self.filteredSets.values(): try: fs.unindex_object(docid) + res += 1 except KeyError: LOG.debug('Attempt to unindex document' ' with id %s failed', docid) - return 1 + + if res > 0: + self._increment_counter() + return 1 + + return 0 + + def _increment_counter(self): + if self._counter is None: + self._counter = Length() + self._counter.change(1) + + def getCounter(self): + """Return a counter which is increased on index changes""" + return self._counter is not None and self._counter() or 0 def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/PluginIndexes/TopicIndex/tests.py b/src/Products/PluginIndexes/TopicIndex/tests.py index f678f687..a6dedf8b 100644 --- a/src/Products/PluginIndexes/TopicIndex/tests.py +++ b/src/Products/PluginIndexes/TopicIndex/tests.py @@ -92,3 +92,23 @@ def testRemoval(self): self.TI.index_object(1, Obj('1', 'doc2')) self._searchOr('doc1', [2]) self._searchOr('doc2', [1, 3, 4]) + + def test_getCounter(self): + index = self.TI + index._counter.set(0) + + self.assertEqual(index.getCounter(), 0) + + index.index_object(1, Obj(1, 'doc1')) + self.assertEqual(index.getCounter(), 1) + + index.unindex_object(1) + self.assertEqual(index.getCounter(), 2) + + # unknown id + index.unindex_object(1) + self.assertEqual(index.getCounter(), 2) + + # clear changes the index + index.clear() + self.assertEqual(index.getCounter(), 3) From a95273e7b92ea97471be34a603f4bb965c1b6779 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Mon, 5 Sep 2016 14:56:33 +0200 Subject: [PATCH 03/18] Add getCounter logic to PathIndex --- .../PluginIndexes/PathIndex/PathIndex.py | 20 ++++++++++++++++++- src/Products/PluginIndexes/PathIndex/tests.py | 20 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/Products/PluginIndexes/PathIndex/PathIndex.py b/src/Products/PluginIndexes/PathIndex/PathIndex.py index 7ce7c4e8..5ce20f66 100644 --- a/src/Products/PluginIndexes/PathIndex/PathIndex.py +++ b/src/Products/PluginIndexes/PathIndex/PathIndex.py @@ -31,6 +31,7 @@ IQueryIndex, ISortIndex, IUniqueValueIndex, + IIndexCounter, ) from Products.PluginIndexes.util import safe_callable from Products.ZCatalog.query import IndexQuery @@ -38,7 +39,8 @@ LOG = getLogger('Zope.PathIndex') -@implementer(IPathIndex, IQueryIndex, IUniqueValueIndex, ISortIndex) +@implementer(IPathIndex, IQueryIndex, IUniqueValueIndex, + ISortIndex, IIndexCounter) class PathIndex(Persistent, SimpleItem): """Index for paths returned by getPhysicalPath. @@ -61,6 +63,7 @@ class PathIndex(Persistent, SimpleItem): operators = ('or', 'and') useOperator = 'or' query_options = ('query', 'level', 'operator') + _counter = None manage_options = ( {'label': 'Settings', 'action': 'manage_main'}, @@ -129,6 +132,7 @@ def index_object(self, docid, obj, threshold=100): for i in range(len(comps)): self.insertEntry(comps[i], docid, i) self._unindex[docid] = path + self._increment_counter() return 1 def unindex_object(self, docid): @@ -156,6 +160,7 @@ def unindex_object(self, docid): 'with id %s failed', docid) self._length.change(-1) + self._increment_counter() del self._unindex[docid] def _apply_index(self, request): @@ -188,6 +193,15 @@ def query_index(self, record, resultset=None): return res return IISet() + def _increment_counter(self): + if self._counter is None: + self._counter = Length() + self._counter.change(1) + + def getCounter(self): + """Return a counter which is increased on index changes""" + return self._counter is not None and self._counter() or 0 + def numObjects(self): """ See IPluggableIndex. """ @@ -205,6 +219,10 @@ def clear(self): self._index = OOBTree() self._unindex = IOBTree() self._length = Length(0) + if self._counter is None: + self._counter = Length(0) + else: + self._increment_counter() # IUniqueValueIndex implementation diff --git a/src/Products/PluginIndexes/PathIndex/tests.py b/src/Products/PluginIndexes/PathIndex/tests.py index 11ed8492..75cb809c 100644 --- a/src/Products/PluginIndexes/PathIndex/tests.py +++ b/src/Products/PluginIndexes/PathIndex/tests.py @@ -533,3 +533,23 @@ def test__search_w_level_0(self): self.assertEqual(list(index._search('bb', 1)), [1]) self.assertEqual(list(index._search('aa/bb', 0)), [1]) self.assertEqual(list(index._search('aa/bb', 1)), []) + + def test_getCounter(self): + index = self._makeOne() + + self.assertEqual(index.getCounter(), 0) + + doc = Dummy('/aa/bb') + index.index_object(1, doc) + self.assertEqual(index.getCounter(), 1) + + index.unindex_object(1) + self.assertEqual(index.getCounter(), 2) + + # unknown id + index.unindex_object(1) + self.assertEqual(index.getCounter(), 2) + + # clear changes the index + index.clear() + self.assertEqual(index.getCounter(), 3) From c817b8da71088d813cc44ef9feae99267f6f17f4 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Mon, 10 Oct 2016 16:32:16 +0200 Subject: [PATCH 04/18] Refactor caching --- src/Products/ZCatalog/Catalog.py | 7 +++---- src/Products/ZCatalog/cache.py | 15 ++++++++++----- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/Products/ZCatalog/Catalog.py b/src/Products/ZCatalog/Catalog.py index 77a2f7ae..6bd3642c 100644 --- a/src/Products/ZCatalog/Catalog.py +++ b/src/Products/ZCatalog/Catalog.py @@ -29,7 +29,6 @@ from Missing import MV from Persistence import Persistent from ZTUtils.Lazy import LazyMap, LazyCat, LazyValues -from plone.memoize import ram from Products.PluginIndexes.interfaces import ( ILimitedResultIndex, @@ -39,7 +38,7 @@ from Products.PluginIndexes.util import safe_callable from Products.ZCatalog.CatalogBrains import AbstractCatalogBrain, NoBrainer from Products.ZCatalog.plan import CatalogPlan -from Products.ZCatalog.cache import _apply_query_plan_cachekey +from Products.ZCatalog.cache import cache from Products.ZCatalog.ProgressHandler import ZLogHandler from Products.ZCatalog.query import IndexQuery @@ -596,9 +595,9 @@ def _search_index(self, cr, index_id, query, rs): return rs - @ram.cache(_apply_query_plan_cachekey) + @cache() def _apply_query_plan(self, cr, query): - + plan = cr.plan() if not plan: plan = self._sorted_search_indexes(query) diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index 6f1b5787..dfc5fad3 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -19,6 +19,7 @@ from Products.PluginIndexes.interfaces import IIndexCounter from Products.PluginIndexes.interfaces import IDateRangeIndex from Products.PluginIndexes.interfaces import IDateIndex +from plone.memoize import ram class CatalogCacheKey(object): @@ -45,7 +46,7 @@ def make_key(self, query): catalog = self.catalog - key = [] + keys = [] for name, value in query.items(): if name in catalog.indexes: index = catalog.getIndex(name) @@ -63,15 +64,15 @@ def make_key(self, query): kvl = [] for k, v in value.items(): v = self._convert_datum(index, v) - v.append((k, v)) - value = sorted(kvl) + kvl.append((k, v)) + value = frozenset(kvl) else: value = self._convert_datum(index, value) - key.append((name, counter, value)) + keys.append((name, counter, value)) - key = tuple(sorted(key)) + key = frozenset(keys) cache_key = '%s-%s' % (self.cid, hash(key)) return cache_key @@ -114,3 +115,7 @@ def _apply_query_plan_cachekey(method, catalog, plan, query): if cc.key is None: raise DontCache return cc.key + + +def cache(): + return ram.cache(_apply_query_plan_cachekey) From e0312b250bfca59f331b68007c582af1c81fcbb2 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Tue, 11 Oct 2016 09:38:32 +0200 Subject: [PATCH 05/18] Implement IIndexCounter --- src/Products/PluginIndexes/unindex.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Products/PluginIndexes/unindex.py b/src/Products/PluginIndexes/unindex.py index 229e90e7..a93dec29 100644 --- a/src/Products/PluginIndexes/unindex.py +++ b/src/Products/PluginIndexes/unindex.py @@ -40,6 +40,7 @@ ISortIndex, IUniqueValueIndex, IRequestCacheIndex, + IIndexCounter, ) from Products.PluginIndexes.util import safe_callable from Products.ZCatalog.query import IndexQuery @@ -49,7 +50,7 @@ @implementer(ILimitedResultIndex, IQueryIndex, IUniqueValueIndex, - ISortIndex, IRequestCacheIndex) + ISortIndex, IRequestCacheIndex, IIndexCounter) class UnIndex(SimpleItem): """Simple forward and reverse index. """ From 1b0b862195377ba826d8516aa01c4c6d37ec5616 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Tue, 11 Oct 2016 09:41:27 +0200 Subject: [PATCH 06/18] Refactor decorator and cache key format --- src/Products/ZCatalog/Catalog.py | 2 +- src/Products/ZCatalog/cache.py | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Products/ZCatalog/Catalog.py b/src/Products/ZCatalog/Catalog.py index 6bd3642c..f73b7466 100644 --- a/src/Products/ZCatalog/Catalog.py +++ b/src/Products/ZCatalog/Catalog.py @@ -595,7 +595,7 @@ def _search_index(self, cr, index_id, query, rs): return rs - @cache() + @cache def _apply_query_plan(self, cr, query): plan = cr.plan() diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index dfc5fad3..87d6cb87 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -31,10 +31,11 @@ def __init__(self, catalog, query=None): self.key = self.make_key(query) def get_id(self): - parent = aq_parent(self.catalog) + catalog = self.catalog + parent = aq_parent(catalog) path = getattr(aq_base(parent), 'getPhysicalPath', None) if path is None: - path = ('', 'NonPersistentCatalog') + path = ('', 'NonPersistentCatalog', id(catalog)) else: path = tuple(parent.getPhysicalPath()) return path @@ -73,7 +74,7 @@ def make_key(self, query): keys.append((name, counter, value)) key = frozenset(keys) - cache_key = '%s-%s' % (self.cid, hash(key)) + cache_key = (self.cid, key) return cache_key def _convert_datum(self, index, value): @@ -117,5 +118,8 @@ def _apply_query_plan_cachekey(method, catalog, plan, query): return cc.key -def cache(): - return ram.cache(_apply_query_plan_cachekey) +def cache(fun): + @ram.cache(_apply_query_plan_cachekey) + def decorator(*args, **kwargs): + return fun(*args, **kwargs) + return decorator From 6d78f86d2c36a4a1bbc1b4bb6b6dec9f6e31c47b Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Tue, 11 Oct 2016 09:46:47 +0200 Subject: [PATCH 07/18] Need unique id of catalog instance for caching support --- src/Products/ZCatalog/tests/test_zcatalog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Products/ZCatalog/tests/test_zcatalog.py b/src/Products/ZCatalog/tests/test_zcatalog.py index 04a50672..11310ed0 100644 --- a/src/Products/ZCatalog/tests/test_zcatalog.py +++ b/src/Products/ZCatalog/tests/test_zcatalog.py @@ -102,7 +102,7 @@ class ZCatalogBase(object): def _makeOne(self): from Products.ZCatalog.ZCatalog import ZCatalog - return ZCatalog('Catalog') + return ZCatalog('Catalog-%s' % id(self)) def _makeOneIndex(self, name): from Products.PluginIndexes.FieldIndex.FieldIndex import FieldIndex From 67c345de778995ea0e15005390508481236b9cd1 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Fri, 14 Oct 2016 17:07:07 +0200 Subject: [PATCH 08/18] Fix catalog test for caching support --- src/Products/ZCatalog/tests/test_catalog.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/Products/ZCatalog/tests/test_catalog.py b/src/Products/ZCatalog/tests/test_catalog.py index 8cfe8c13..7ecf9ca7 100644 --- a/src/Products/ZCatalog/tests/test_catalog.py +++ b/src/Products/ZCatalog/tests/test_catalog.py @@ -12,6 +12,7 @@ ############################################################################## import unittest +from zope.testing import cleanup from itertools import chain import random @@ -32,6 +33,9 @@ def __init__(self, num): def title(self): return '{0:d}'.format(self.num) + def getPhysicalPath(self): + return ['/', self.title] + class Dummy(ExtensionClass.Base): @@ -389,7 +393,7 @@ def test_keyword_index_length(self): self.assertEqual(len(a), 0) -class TestCatalogSortBatch(unittest.TestCase): +class TestCatalogSortBatch(cleanup.CleanUp, unittest.TestCase): upper = 100 @@ -960,7 +964,7 @@ def test_range_search(self): ) -class TestMergeResults(unittest.TestCase): +class TestMergeResults(cleanup.CleanUp, unittest.TestCase): def _make_one(self): from Products.ZCatalog.Catalog import Catalog @@ -969,16 +973,17 @@ def _make_one(self): def _make_many(self): from Products.ZCatalog.Catalog import mergeResults catalogs = [] - for i in range(3): + for ci in range(3): cat = self._make_one() cat.lexicon = PLexicon('lexicon') cat.addIndex('num', FieldIndex('num')) cat.addIndex('big', FieldIndex('big')) cat.addIndex('number', FieldIndex('number')) - i = ZCTextIndex('title', caller=cat, index_factory=OkapiIndex, - lexicon_id='lexicon') - cat.addIndex('title', i) - cat = cat.__of__(ZDummy(16336)) + text_index = ZCTextIndex('title', caller=cat, + index_factory=OkapiIndex, + lexicon_id='lexicon') + cat.addIndex('title', text_index) + cat = cat.__of__(ZDummy(16336 + ci)) for i in range(10): obj = ZDummy(i) obj.big = i > 5 From c52d18abb0a826ecfbf8edbb8397f6625ca95167 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Fri, 14 Oct 2016 17:07:45 +0200 Subject: [PATCH 09/18] Initial cache tests --- src/Products/ZCatalog/cache.py | 15 +++- src/Products/ZCatalog/tests/test_cache.py | 102 ++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 src/Products/ZCatalog/tests/test_cache.py diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index 87d6cb87..07df9224 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -35,7 +35,7 @@ def get_id(self): parent = aq_parent(catalog) path = getattr(aq_base(parent), 'getPhysicalPath', None) if path is None: - path = ('', 'NonPersistentCatalog', id(catalog)) + path = ('', 'NonPersistentCatalog') else: path = tuple(parent.getPhysicalPath()) return path @@ -123,3 +123,16 @@ def cache(fun): def decorator(*args, **kwargs): return fun(*args, **kwargs) return decorator + + +# Make sure we provide test isolation, works only for ramcache +def _cache_clear(): + cache_adapter = ram.store_in_cache(_apply_query_plan_cachekey) + if hasattr(cache_adapter, 'ramcache'): + cache_adapter.ramcache.invalidateAll() + else: + raise AttributeError('Only ramcache supported for testing') + +from zope.testing.cleanup import addCleanUp # NOQA +addCleanUp(_cache_clear) +del addCleanUp diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py new file mode 100644 index 00000000..4d3d2348 --- /dev/null +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -0,0 +1,102 @@ +############################################################################## +# +# Copyright (c) 2010 Zope Foundation and Contributors. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +############################################################################## + +import unittest + +from zope.testing import cleanup + +from Products.PluginIndexes.BooleanIndex.BooleanIndex import BooleanIndex +from Products.PluginIndexes.DateRangeIndex.DateRangeIndex import DateRangeIndex +from Products.PluginIndexes.DateIndex.DateIndex import DateIndex +from Products.PluginIndexes.FieldIndex.FieldIndex import FieldIndex +from Products.PluginIndexes.KeywordIndex.KeywordIndex import KeywordIndex +from Products.PluginIndexes.PathIndex.PathIndex import PathIndex +from Products.PluginIndexes.UUIDIndex.UUIDIndex import UUIDIndex +from Products.ZCatalog.Catalog import Catalog +from Products.ZCatalog.ZCatalog import ZCatalog +from Products.ZCatalog.cache import CatalogCacheKey + + +class Dummy(object): + + def __init__(self, num): + self.num = num + + def big(self): + return self.num > 5 + + def numbers(self): + return (self.num, self.num + 1) + + def getPhysicalPath(self): + return '/%s' % self.num + + def start(self): + return '2013-07-%.2d' % (self.num + 1) + + def end(self): + return '2013-07-%.2d' % (self.num + 2) + + +class TestCatalogCacheKey(unittest.TestCase): + + def setUp(self): + self.cat = Catalog('catalog') + + def _makeOne(self, catalog=None, query=None): + if catalog is None: + catalog = self.cat + return CatalogCacheKey(catalog, query=query) + + def test_get_id(self): + cache_key = self._makeOne() + self.assertEquals(cache_key.get_id(), + ('', 'NonPersistentCatalog')) + + def test_get_id_persistent(self): + zcat = ZCatalog('catalog') + cache_key = self._makeOne(zcat._catalog) + self.assertEquals(cache_key.get_id(), ('catalog', )) + + +class TestCatalogQueryKey(cleanup.CleanUp, unittest.TestCase): + + def setUp(self): + cleanup.CleanUp.setUp(self) + zcat = ZCatalog('catalog') + zcat._catalog.addIndex('big', BooleanIndex('big')) + zcat._catalog.addIndex('start', DateIndex('start')) + zcat._catalog.addIndex('date', DateRangeIndex('date', 'start', 'end')) + zcat._catalog.addIndex('num', FieldIndex('num')) + zcat._catalog.addIndex('numbers', KeywordIndex('numbers')) + zcat._catalog.addIndex('path', PathIndex('getPhysicalPath')) + zcat._catalog.addIndex('uuid', UUIDIndex('num')) + self.length = length = 9 + for i in range(length): + obj = Dummy(i) + zcat.catalog_object(obj, str(i)) + + self.zcat = zcat + + def _get_cache_key(self, query=None): + catalog = self.zcat._catalog + return CatalogCacheKey(catalog, query=query).key + + def test_make_key(self): + query = {'big': True} + key = (('catalog',), frozenset([('big', self.length, (True,))])) + self.assertEquals(self._get_cache_key(query), key) + +#class TestCatalogCaching(unittest.TestCase): +# +# def test_caching From a4d0f334864b05111867f5fb354225498a21320e Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Mon, 17 Oct 2016 14:46:48 +0200 Subject: [PATCH 10/18] Add more caching tests --- src/Products/ZCatalog/cache.py | 9 ++- src/Products/ZCatalog/tests/test_cache.py | 76 ++++++++++++++++++-- src/Products/ZCatalog/tests/test_zcatalog.py | 6 +- 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index 07df9224..b612759d 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -126,13 +126,18 @@ def decorator(*args, **kwargs): # Make sure we provide test isolation, works only for ramcache -def _cache_clear(): +def _get_cache(): cache_adapter = ram.store_in_cache(_apply_query_plan_cachekey) if hasattr(cache_adapter, 'ramcache'): - cache_adapter.ramcache.invalidateAll() + return cache_adapter.ramcache else: raise AttributeError('Only ramcache supported for testing') + +def _cache_clear(): + ram_cache = _get_cache() + ram_cache.invalidateAll() + from zope.testing.cleanup import addCleanUp # NOQA addCleanUp(_cache_clear) del addCleanUp diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 4d3d2348..ce94396b 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -25,6 +25,7 @@ from Products.ZCatalog.Catalog import Catalog from Products.ZCatalog.ZCatalog import ZCatalog from Products.ZCatalog.cache import CatalogCacheKey +from Products.ZCatalog.cache import _get_cache class Dummy(object): @@ -88,15 +89,82 @@ def setUp(self): self.zcat = zcat + def _apply_query(self, query): + cache = _get_cache() + + res1 = self.zcat.search(query) + stats = cache.getStatistics() + + hits = stats[0]['hits'] + misses = stats[0]['misses'] + + res2 = self.zcat.search(query) + stats = cache.getStatistics() + + # check if chache hits + self.assertEqual(stats[0]['hits'], hits + 1) + self.assertEqual(stats[0]['misses'], misses) + + # compare result + rset1 = map(lambda x: x.getRID(), res1) + rset2 = map(lambda x: x.getRID(), res2) + self.assertEqual(rset1, rset2) + def _get_cache_key(self, query=None): catalog = self.zcat._catalog + query = catalog.make_query(query) return CatalogCacheKey(catalog, query=query).key def test_make_key(self): query = {'big': True} - key = (('catalog',), frozenset([('big', self.length, (True,))])) + key = (('catalog',), + frozenset([('big', self.length, (True,))])) self.assertEquals(self._get_cache_key(query), key) -#class TestCatalogCaching(unittest.TestCase): -# -# def test_caching + query = {'start': '2013-07-01'} + key = (('catalog',), + frozenset([('start', self.length, ('2013-07-01',))])) + self.assertEquals(self._get_cache_key(query), key) + + query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} + key = (('catalog',), + frozenset([('date', 9, ('2013-07-05',)), + ('numbers', 9, (1, 3)), + ('path', 9, ('/1',))])) + self.assertEquals(self._get_cache_key(query), key) + + def test_cache(self): + query = {'big': True} + self._apply_query(query) + + query = {'start': '2013-07-01'} + self._apply_query(query) + + query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} + self._apply_query(query) + + def test_cache_invalidate(self): + cache = _get_cache() + query = {'big': False} + + res1 = self.zcat.search(query) + stats = cache.getStatistics() + + hits = stats[0]['hits'] + misses = stats[0]['misses'] + + # catalog new object + obj = Dummy(20) + self.zcat.catalog_object(obj, str(20)) + + res2 = self.zcat.search(query) + stats = cache.getStatistics() + + # check if chache misses + self.assertEqual(stats[0]['hits'], hits) + self.assertEqual(stats[0]['misses'], misses + 1) + + # compare result + rset1 = map(lambda x: x.getRID(), res1) + rset2 = map(lambda x: x.getRID(), res2) + self.assertEqual(rset1, rset2) diff --git a/src/Products/ZCatalog/tests/test_zcatalog.py b/src/Products/ZCatalog/tests/test_zcatalog.py index 11310ed0..2937eb9e 100644 --- a/src/Products/ZCatalog/tests/test_zcatalog.py +++ b/src/Products/ZCatalog/tests/test_zcatalog.py @@ -12,6 +12,7 @@ ############################################################################## import unittest +from zope.testing import cleanup from AccessControl.SecurityManagement import setSecurityManager from AccessControl.SecurityManagement import noSecurityManager @@ -98,17 +99,18 @@ def validate(self, accessed, container, name, value): raise Unauthorized(name) -class ZCatalogBase(object): +class ZCatalogBase(cleanup.CleanUp): def _makeOne(self): from Products.ZCatalog.ZCatalog import ZCatalog - return ZCatalog('Catalog-%s' % id(self)) + return ZCatalog('Catalog') def _makeOneIndex(self, name): from Products.PluginIndexes.FieldIndex.FieldIndex import FieldIndex return FieldIndex(name) def setUp(self): + cleanup.CleanUp.setUp(self) self._catalog = self._makeOne() def tearDown(self): From 1dc26e12488673a232f088528e014f8501a0fb05 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Tue, 18 Oct 2016 10:01:07 +0200 Subject: [PATCH 11/18] Cache key is invariant of sort or pagenation options --- src/Products/ZCatalog/cache.py | 17 +++++++++++- src/Products/ZCatalog/tests/test_cache.py | 33 +++++++++++++++-------- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index b612759d..f39a3995 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -47,6 +47,17 @@ def make_key(self, query): catalog = self.catalog + def skip(name, value): + if name in ['b_start', 'b_size']: + return True + elif catalog._get_sort_attr('on', {name: value}): + return True + elif catalog._get_sort_attr('limit', {name: value}): + return True + elif catalog._get_sort_attr('order', {name: value}): + return True + return False + keys = [] for name, value in query.items(): if name in catalog.indexes: @@ -57,6 +68,10 @@ def make_key(self, query): # cache key invalidation cannot be supported if # any index of query cannot be tested for changes return None + elif skip(name, value): + # applying the query to indexes is invariant of + # sort or pagination options + continue else: # return None if query has a nonexistent index key return None @@ -71,7 +86,7 @@ def make_key(self, query): else: value = self._convert_datum(index, value) - keys.append((name, counter, value)) + keys.append((name, value, counter)) key = frozenset(keys) cache_key = (self.cid, key) diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index ce94396b..14603617 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -117,21 +117,32 @@ def _get_cache_key(self, query=None): def test_make_key(self): query = {'big': True} - key = (('catalog',), - frozenset([('big', self.length, (True,))])) - self.assertEquals(self._get_cache_key(query), key) + expect = (('catalog',), + frozenset([('big', (True,), self.length)])) + self.assertEquals(self._get_cache_key(query), expect) query = {'start': '2013-07-01'} - key = (('catalog',), - frozenset([('start', self.length, ('2013-07-01',))])) - self.assertEquals(self._get_cache_key(query), key) + expect = (('catalog',), + frozenset([('start', ('2013-07-01',), self.length)])) + self.assertEquals(self._get_cache_key(query), expect) query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} - key = (('catalog',), - frozenset([('date', 9, ('2013-07-05',)), - ('numbers', 9, (1, 3)), - ('path', 9, ('/1',))])) - self.assertEquals(self._get_cache_key(query), key) + expect = (('catalog',), + frozenset([('date', ('2013-07-05',), self.length), + ('numbers', (1, 3), self.length), + ('path', ('/1',), self.length)])) + self.assertEquals(self._get_cache_key(query), expect) + + queries = [{'big': True, 'b_start': 0}, + {'big': True, 'b_start': 0, 'b_size': 5}, + {'big': True, 'sort_on': 'big'}, + {'big': True, 'sort_on': 'big', 'sort_limit': 3}, + {'big': True, 'sort_on': 'big', 'sort_order': 'descending'}, + ] + expect = (('catalog',), + frozenset([('big', (True,), self.length)])) + for query in queries: + self.assertEquals(self._get_cache_key(query), expect) def test_cache(self): query = {'big': True} From 3b8914dcf0897d6baf057b35ba0ad8f946a821bb Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Tue, 3 Apr 2018 23:18:28 +0200 Subject: [PATCH 12/18] wrap map calls in list calls --- src/Products/ZCatalog/tests/test_cache.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 14603617..2b10685e 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -106,8 +106,8 @@ def _apply_query(self, query): self.assertEqual(stats[0]['misses'], misses) # compare result - rset1 = map(lambda x: x.getRID(), res1) - rset2 = map(lambda x: x.getRID(), res2) + rset1 = list(map(lambda x: x.getRID(), res1)) + rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) def _get_cache_key(self, query=None): @@ -176,6 +176,6 @@ def test_cache_invalidate(self): self.assertEqual(stats[0]['misses'], misses + 1) # compare result - rset1 = map(lambda x: x.getRID(), res1) - rset2 = map(lambda x: x.getRID(), res2) + rset1 = list(map(lambda x: x.getRID(), res1)) + rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) From ee06fb9751c987ebfff6ae45ba1673d3acdc7b07 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Fri, 6 Apr 2018 14:05:45 +0200 Subject: [PATCH 13/18] Add transaction awareness for cache keys --- .../DateRangeIndex/DateRangeIndex.py | 1 + .../PluginIndexes/PathIndex/PathIndex.py | 13 ++++++++ .../PluginIndexes/TopicIndex/TopicIndex.py | 11 +++++++ src/Products/PluginIndexes/interfaces.py | 3 ++ .../PluginIndexes/tests/test_unindex.py | 3 ++ src/Products/PluginIndexes/unindex.py | 13 ++++++++ src/Products/ZCatalog/cache.py | 4 +-- src/Products/ZCatalog/tests/test_cache.py | 31 +++++++++++++++---- 8 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/Products/PluginIndexes/DateRangeIndex/DateRangeIndex.py b/src/Products/PluginIndexes/DateRangeIndex/DateRangeIndex.py index 78724e9e..64990b70 100644 --- a/src/Products/PluginIndexes/DateRangeIndex/DateRangeIndex.py +++ b/src/Products/PluginIndexes/DateRangeIndex/DateRangeIndex.py @@ -250,6 +250,7 @@ def getRequestCacheKey(self, record, resultset=None): # unique index identifier iid = '_{0}_{1}_{2}'.format(self.__class__.__name__, self.id, self.getCounter()) + # record identifier if resultset is None: rid = '_{0}'.format(tid) diff --git a/src/Products/PluginIndexes/PathIndex/PathIndex.py b/src/Products/PluginIndexes/PathIndex/PathIndex.py index 5ce20f66..c27718f6 100644 --- a/src/Products/PluginIndexes/PathIndex/PathIndex.py +++ b/src/Products/PluginIndexes/PathIndex/PathIndex.py @@ -24,8 +24,10 @@ from BTrees.OOBTree import OOBTree from BTrees.Length import Length from Persistence import Persistent +from ZODB.utils import newTid from zope.interface import implementer + from Products.PluginIndexes.interfaces import ( IPathIndex, IQueryIndex, @@ -202,6 +204,17 @@ def getCounter(self): """Return a counter which is increased on index changes""" return self._counter is not None and self._counter() or 0 + def getCounterKey(self): + """Returns an unique key indicating an uniqe state of the index""" + if self._counter is not None: + key = (self.getCounter(), self._counter._p_serial) + else: + # generate new serial for backward compatibility + # if counter is not set + key = (self.getCounter(), newTid(None)) + + return key + def numObjects(self): """ See IPluggableIndex. """ diff --git a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py index 54c1a431..ff0cce8b 100644 --- a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py +++ b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py @@ -21,6 +21,7 @@ from BTrees.Length import Length from OFS.SimpleItem import SimpleItem from Persistence import Persistent +from ZODB.utils import newTid from zope.interface import implementer from Products.PluginIndexes.interfaces import ( @@ -105,6 +106,16 @@ def getCounter(self): """Return a counter which is increased on index changes""" return self._counter is not None and self._counter() or 0 + def getCounterKey(self): + """Returns an unique key indicating an uniqe state of the index""" + if self._counter is not None: + key = (self.getCounter(), self._counter._p_serial) + else: + # counter is not set, generate new serial + key = (self.getCounter(), newTid(None)) + + return key + def numObjects(self): """Return the number of indexed objects.""" return 'n/a' diff --git a/src/Products/PluginIndexes/interfaces.py b/src/Products/PluginIndexes/interfaces.py index 27dd9576..665032e4 100644 --- a/src/Products/PluginIndexes/interfaces.py +++ b/src/Products/PluginIndexes/interfaces.py @@ -290,3 +290,6 @@ class IIndexCounter(Interface): def getCounter(): """Return a counter which is increased on index changes""" + + def getCounterKey(): + """Returns an unique key indicating an uniqe state of the index""" diff --git a/src/Products/PluginIndexes/tests/test_unindex.py b/src/Products/PluginIndexes/tests/test_unindex.py index 66eeefef..22abce8e 100644 --- a/src/Products/PluginIndexes/tests/test_unindex.py +++ b/src/Products/PluginIndexes/tests/test_unindex.py @@ -16,6 +16,9 @@ from BTrees.IIBTree import difference from OFS.SimpleItem import SimpleItem from Testing.makerequest import makerequest +from Acquisition import aq_base +import ZODB +import transaction from Products.ZCatalog.query import IndexQuery diff --git a/src/Products/PluginIndexes/unindex.py b/src/Products/PluginIndexes/unindex.py index a93dec29..6e988099 100644 --- a/src/Products/PluginIndexes/unindex.py +++ b/src/Products/PluginIndexes/unindex.py @@ -31,6 +31,7 @@ from BTrees.OOBTree import OOBTree from OFS.SimpleItem import SimpleItem from ZODB.POSException import ConflictError +from ZODB.utils import newTid from zope.interface import implementer from Products.PluginIndexes.cache import RequestCache @@ -310,6 +311,17 @@ def getCounter(self): """Return a counter which is increased on index changes""" return self._counter is not None and self._counter() or 0 + def getCounterKey(self): + """Returns an unique key indicating an uniqe state of the index""" + if self._counter is not None: + key = (self.getCounter(), self._counter._p_serial) + else: + # generate new serial for backward compatibility + # if counter is not set + key = (self.getCounter(), newTid(None)) + + return key + def numObjects(self): """Return the number of indexed objects.""" return len(self._unindex) @@ -405,6 +417,7 @@ def getRequestCacheKey(self, record, resultset=None): # unique index identifier iid = '_{0}_{1}_{2}'.format(self.__class__.__name__, self.id, self.getCounter()) + return (iid, rid) def _apply_index(self, request, resultset=None): diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index f39a3995..82f5d5f1 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -63,7 +63,7 @@ def skip(name, value): if name in catalog.indexes: index = catalog.getIndex(name) if IIndexCounter.providedBy(index): - counter = index.getCounter() + ck = index.getCounterKey() else: # cache key invalidation cannot be supported if # any index of query cannot be tested for changes @@ -86,7 +86,7 @@ def skip(name, value): else: value = self._convert_datum(index, value) - keys.append((name, value, counter)) + keys.append((name, value, ck)) key = frozenset(keys) cache_key = (self.cid, key) diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 2b10685e..2c9a905e 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -118,19 +118,35 @@ def _get_cache_key(self, query=None): def test_make_key(self): query = {'big': True} expect = (('catalog',), - frozenset([('big', (True,), self.length)])) + frozenset([('big', (True,), + (self.length, + b'\x00\x00\x00\x00\x00\x00\x00\x00'))]) + ) + self.assertEquals(self._get_cache_key(query), expect) query = {'start': '2013-07-01'} expect = (('catalog',), - frozenset([('start', ('2013-07-01',), self.length)])) + frozenset([('start', ('2013-07-01',), + (self.length, + b'\x00\x00\x00\x00\x00\x00\x00\x00'))]) + + ) + self.assertEquals(self._get_cache_key(query), expect) query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} expect = (('catalog',), - frozenset([('date', ('2013-07-05',), self.length), - ('numbers', (1, 3), self.length), - ('path', ('/1',), self.length)])) + frozenset([('date', ('2013-07-05',), + (self.length, + b'\x00\x00\x00\x00\x00\x00\x00\x00')), + ('numbers', (1, 3), + (self.length, + b'\x00\x00\x00\x00\x00\x00\x00\x00')), + ('path', ('/1',), + (self.length, + b'\x00\x00\x00\x00\x00\x00\x00\x00'))])) + self.assertEquals(self._get_cache_key(query), expect) queries = [{'big': True, 'b_start': 0}, @@ -140,7 +156,10 @@ def test_make_key(self): {'big': True, 'sort_on': 'big', 'sort_order': 'descending'}, ] expect = (('catalog',), - frozenset([('big', (True,), self.length)])) + frozenset([('big', (True,), + (self.length, + b'\x00\x00\x00\x00\x00\x00\x00\x00'))])) + for query in queries: self.assertEquals(self._get_cache_key(query), expect) From 6a39e7ee7ee3507d2de385bab06c70e7d1e8f87a Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Tue, 10 Apr 2018 21:30:32 +0200 Subject: [PATCH 14/18] Revert "Add transaction awareness for cache keys" because _p_serial isn't an adequate marker for an unique key generation This reverts commit 9d8aea653c6a729085e41bf48f0fe69cd6ca8f74. --- .../PluginIndexes/PathIndex/PathIndex.py | 13 -------- .../PluginIndexes/TopicIndex/TopicIndex.py | 11 ------- src/Products/PluginIndexes/interfaces.py | 3 -- .../PluginIndexes/tests/test_unindex.py | 3 -- src/Products/PluginIndexes/unindex.py | 12 ------- src/Products/ZCatalog/cache.py | 4 +-- src/Products/ZCatalog/tests/test_cache.py | 31 ++++--------------- 7 files changed, 8 insertions(+), 69 deletions(-) diff --git a/src/Products/PluginIndexes/PathIndex/PathIndex.py b/src/Products/PluginIndexes/PathIndex/PathIndex.py index c27718f6..5ce20f66 100644 --- a/src/Products/PluginIndexes/PathIndex/PathIndex.py +++ b/src/Products/PluginIndexes/PathIndex/PathIndex.py @@ -24,10 +24,8 @@ from BTrees.OOBTree import OOBTree from BTrees.Length import Length from Persistence import Persistent -from ZODB.utils import newTid from zope.interface import implementer - from Products.PluginIndexes.interfaces import ( IPathIndex, IQueryIndex, @@ -204,17 +202,6 @@ def getCounter(self): """Return a counter which is increased on index changes""" return self._counter is not None and self._counter() or 0 - def getCounterKey(self): - """Returns an unique key indicating an uniqe state of the index""" - if self._counter is not None: - key = (self.getCounter(), self._counter._p_serial) - else: - # generate new serial for backward compatibility - # if counter is not set - key = (self.getCounter(), newTid(None)) - - return key - def numObjects(self): """ See IPluggableIndex. """ diff --git a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py index ff0cce8b..54c1a431 100644 --- a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py +++ b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py @@ -21,7 +21,6 @@ from BTrees.Length import Length from OFS.SimpleItem import SimpleItem from Persistence import Persistent -from ZODB.utils import newTid from zope.interface import implementer from Products.PluginIndexes.interfaces import ( @@ -106,16 +105,6 @@ def getCounter(self): """Return a counter which is increased on index changes""" return self._counter is not None and self._counter() or 0 - def getCounterKey(self): - """Returns an unique key indicating an uniqe state of the index""" - if self._counter is not None: - key = (self.getCounter(), self._counter._p_serial) - else: - # counter is not set, generate new serial - key = (self.getCounter(), newTid(None)) - - return key - def numObjects(self): """Return the number of indexed objects.""" return 'n/a' diff --git a/src/Products/PluginIndexes/interfaces.py b/src/Products/PluginIndexes/interfaces.py index 665032e4..27dd9576 100644 --- a/src/Products/PluginIndexes/interfaces.py +++ b/src/Products/PluginIndexes/interfaces.py @@ -290,6 +290,3 @@ class IIndexCounter(Interface): def getCounter(): """Return a counter which is increased on index changes""" - - def getCounterKey(): - """Returns an unique key indicating an uniqe state of the index""" diff --git a/src/Products/PluginIndexes/tests/test_unindex.py b/src/Products/PluginIndexes/tests/test_unindex.py index 22abce8e..66eeefef 100644 --- a/src/Products/PluginIndexes/tests/test_unindex.py +++ b/src/Products/PluginIndexes/tests/test_unindex.py @@ -16,9 +16,6 @@ from BTrees.IIBTree import difference from OFS.SimpleItem import SimpleItem from Testing.makerequest import makerequest -from Acquisition import aq_base -import ZODB -import transaction from Products.ZCatalog.query import IndexQuery diff --git a/src/Products/PluginIndexes/unindex.py b/src/Products/PluginIndexes/unindex.py index 6e988099..20f00128 100644 --- a/src/Products/PluginIndexes/unindex.py +++ b/src/Products/PluginIndexes/unindex.py @@ -31,7 +31,6 @@ from BTrees.OOBTree import OOBTree from OFS.SimpleItem import SimpleItem from ZODB.POSException import ConflictError -from ZODB.utils import newTid from zope.interface import implementer from Products.PluginIndexes.cache import RequestCache @@ -311,17 +310,6 @@ def getCounter(self): """Return a counter which is increased on index changes""" return self._counter is not None and self._counter() or 0 - def getCounterKey(self): - """Returns an unique key indicating an uniqe state of the index""" - if self._counter is not None: - key = (self.getCounter(), self._counter._p_serial) - else: - # generate new serial for backward compatibility - # if counter is not set - key = (self.getCounter(), newTid(None)) - - return key - def numObjects(self): """Return the number of indexed objects.""" return len(self._unindex) diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index 82f5d5f1..f39a3995 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -63,7 +63,7 @@ def skip(name, value): if name in catalog.indexes: index = catalog.getIndex(name) if IIndexCounter.providedBy(index): - ck = index.getCounterKey() + counter = index.getCounter() else: # cache key invalidation cannot be supported if # any index of query cannot be tested for changes @@ -86,7 +86,7 @@ def skip(name, value): else: value = self._convert_datum(index, value) - keys.append((name, value, ck)) + keys.append((name, value, counter)) key = frozenset(keys) cache_key = (self.cid, key) diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 2c9a905e..2b10685e 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -118,35 +118,19 @@ def _get_cache_key(self, query=None): def test_make_key(self): query = {'big': True} expect = (('catalog',), - frozenset([('big', (True,), - (self.length, - b'\x00\x00\x00\x00\x00\x00\x00\x00'))]) - ) - + frozenset([('big', (True,), self.length)])) self.assertEquals(self._get_cache_key(query), expect) query = {'start': '2013-07-01'} expect = (('catalog',), - frozenset([('start', ('2013-07-01',), - (self.length, - b'\x00\x00\x00\x00\x00\x00\x00\x00'))]) - - ) - + frozenset([('start', ('2013-07-01',), self.length)])) self.assertEquals(self._get_cache_key(query), expect) query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} expect = (('catalog',), - frozenset([('date', ('2013-07-05',), - (self.length, - b'\x00\x00\x00\x00\x00\x00\x00\x00')), - ('numbers', (1, 3), - (self.length, - b'\x00\x00\x00\x00\x00\x00\x00\x00')), - ('path', ('/1',), - (self.length, - b'\x00\x00\x00\x00\x00\x00\x00\x00'))])) - + frozenset([('date', ('2013-07-05',), self.length), + ('numbers', (1, 3), self.length), + ('path', ('/1',), self.length)])) self.assertEquals(self._get_cache_key(query), expect) queries = [{'big': True, 'b_start': 0}, @@ -156,10 +140,7 @@ def test_make_key(self): {'big': True, 'sort_on': 'big', 'sort_order': 'descending'}, ] expect = (('catalog',), - frozenset([('big', (True,), - (self.length, - b'\x00\x00\x00\x00\x00\x00\x00\x00'))])) - + frozenset([('big', (True,), self.length)])) for query in queries: self.assertEquals(self._get_cache_key(query), expect) From 26931ccef4c44ab41449e22200a543fd644ea6ae Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Sun, 15 Apr 2018 22:45:32 +0200 Subject: [PATCH 15/18] Fixed inconsistent cache read of concurrent threads --- .../PluginIndexes/BooleanIndex/tests.py | 10 +- src/Products/PluginIndexes/DateIndex/tests.py | 10 +- .../PluginIndexes/DateRangeIndex/tests.py | 10 +- .../PluginIndexes/KeywordIndex/tests.py | 10 +- .../PluginIndexes/PathIndex/PathIndex.py | 6 +- src/Products/PluginIndexes/PathIndex/tests.py | 10 +- .../PluginIndexes/TopicIndex/TopicIndex.py | 6 +- .../PluginIndexes/TopicIndex/tests.py | 10 +- src/Products/PluginIndexes/UUIDIndex/tests.py | 10 +- .../PluginIndexes/tests/test_unindex.py | 12 +- src/Products/PluginIndexes/unindex.py | 6 +- src/Products/ZCatalog/cache.py | 6 +- src/Products/ZCatalog/tests/test_cache.py | 118 ++++++++++++++++-- 13 files changed, 170 insertions(+), 54 deletions(-) diff --git a/src/Products/PluginIndexes/BooleanIndex/tests.py b/src/Products/PluginIndexes/BooleanIndex/tests.py index c5208190..0a50834a 100644 --- a/src/Products/PluginIndexes/BooleanIndex/tests.py +++ b/src/Products/PluginIndexes/BooleanIndex/tests.py @@ -257,19 +257,19 @@ def test_reindexation_when_index_reversed(self): def test_getCounter(self): index = self._makeOne() - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) obj = Dummy(1, True) index.index_object(obj.id, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(obj.id) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/DateIndex/tests.py b/src/Products/PluginIndexes/DateIndex/tests.py index 274b8b88..9c434fe3 100644 --- a/src/Products/PluginIndexes/DateIndex/tests.py +++ b/src/Products/PluginIndexes/DateIndex/tests.py @@ -337,21 +337,21 @@ def test_getCounter(self): from DateTime import DateTime index = self._makeOne() - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) index.index_object(1, Dummy('b', DateTime(0))) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) def test_precision(self): from DateTime import DateTime diff --git a/src/Products/PluginIndexes/DateRangeIndex/tests.py b/src/Products/PluginIndexes/DateRangeIndex/tests.py index 36fffc6e..5f9cb75f 100644 --- a/src/Products/PluginIndexes/DateRangeIndex/tests.py +++ b/src/Products/PluginIndexes/DateRangeIndex/tests.py @@ -299,22 +299,22 @@ def test_resultset(self): def test_getCounter(self): index = self._makeOne('work', 'start', 'stop') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) k, obj = dummies[0] index.index_object(k, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(k) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) def test_precision(self): precision = 5 diff --git a/src/Products/PluginIndexes/KeywordIndex/tests.py b/src/Products/PluginIndexes/KeywordIndex/tests.py index 44216913..898c2a8e 100644 --- a/src/Products/PluginIndexes/KeywordIndex/tests.py +++ b/src/Products/PluginIndexes/KeywordIndex/tests.py @@ -275,19 +275,19 @@ def test_value_removes(self): def test_getCounter(self): index = self._makeOne('foo') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) obj = Dummy(['hello']) index.index_object(10, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(10) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/PathIndex/PathIndex.py b/src/Products/PluginIndexes/PathIndex/PathIndex.py index 5ce20f66..bce4b728 100644 --- a/src/Products/PluginIndexes/PathIndex/PathIndex.py +++ b/src/Products/PluginIndexes/PathIndex/PathIndex.py @@ -200,7 +200,11 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - return self._counter is not None and self._counter() or 0 + counter = self._counter is not None and self._counter() or 0 + changed = (self._counter is not None and + self._counter._p_changed or False) + + return (counter, changed) def numObjects(self): """ See IPluggableIndex. diff --git a/src/Products/PluginIndexes/PathIndex/tests.py b/src/Products/PluginIndexes/PathIndex/tests.py index 75cb809c..f6025417 100644 --- a/src/Products/PluginIndexes/PathIndex/tests.py +++ b/src/Products/PluginIndexes/PathIndex/tests.py @@ -537,19 +537,19 @@ def test__search_w_level_0(self): def test_getCounter(self): index = self._makeOne() - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) doc = Dummy('/aa/bb') index.index_object(1, doc) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py index 54c1a431..80c91e83 100644 --- a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py +++ b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py @@ -103,7 +103,11 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - return self._counter is not None and self._counter() or 0 + counter = self._counter is not None and self._counter() or 0 + changed = (self._counter is not None and + self._counter._p_changed or False) + + return (counter, changed) def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/PluginIndexes/TopicIndex/tests.py b/src/Products/PluginIndexes/TopicIndex/tests.py index a6dedf8b..b2a7fcfb 100644 --- a/src/Products/PluginIndexes/TopicIndex/tests.py +++ b/src/Products/PluginIndexes/TopicIndex/tests.py @@ -97,18 +97,18 @@ def test_getCounter(self): index = self.TI index._counter.set(0) - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) index.index_object(1, Obj(1, 'doc1')) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/UUIDIndex/tests.py b/src/Products/PluginIndexes/UUIDIndex/tests.py index e7dee9c5..c2089469 100644 --- a/src/Products/PluginIndexes/UUIDIndex/tests.py +++ b/src/Products/PluginIndexes/UUIDIndex/tests.py @@ -179,19 +179,19 @@ def test_non_unique(self): def test_getCounter(self): index = self._makeOne('foo') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) obj = Dummy('a') index.index_object(10, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(10) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/tests/test_unindex.py b/src/Products/PluginIndexes/tests/test_unindex.py index 66eeefef..37acbc5a 100644 --- a/src/Products/PluginIndexes/tests/test_unindex.py +++ b/src/Products/PluginIndexes/tests/test_unindex.py @@ -27,6 +27,7 @@ def _getTargetClass(self): return UnIndex def _makeOne(self, *args, **kw): + index = self._getTargetClass()(*args, **kw) class DummyZCatalog(SimpleItem): @@ -144,9 +145,10 @@ def testQuery(record, expect=1): testQuery(record) def test_getCounter(self): + index = self._makeOne('counter') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) class Dummy(object): id = 1 @@ -154,18 +156,18 @@ class Dummy(object): obj = Dummy() index.index_object(obj.id, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(obj.id) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) def test_no_type_error(self): ''' Check that on Python 3.6 we do not get a TypeError when trying diff --git a/src/Products/PluginIndexes/unindex.py b/src/Products/PluginIndexes/unindex.py index 20f00128..b6d5b761 100644 --- a/src/Products/PluginIndexes/unindex.py +++ b/src/Products/PluginIndexes/unindex.py @@ -308,7 +308,11 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - return self._counter is not None and self._counter() or 0 + counter = self._counter is not None and self._counter() or 0 + changed = (self._counter is not None and + self._counter._p_changed or False) + + return (counter, changed) def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index f39a3995..f44988b4 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -63,7 +63,11 @@ def skip(name, value): if name in catalog.indexes: index = catalog.getIndex(name) if IIndexCounter.providedBy(index): - counter = index.getCounter() + counter, changed = index.getCounter() + + # cannot cache if index was updated + if changed: + return None else: # cache key invalidation cannot be supported if # any index of query cannot be tested for changes diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 2b10685e..48ca5ec9 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -12,8 +12,12 @@ ############################################################################## import unittest +import transaction from zope.testing import cleanup +from ZODB.tests.test_storage import MinimalMemoryStorage +from ZODB import DB +from ZODB.POSException import ConflictError from Products.PluginIndexes.BooleanIndex.BooleanIndex import BooleanIndex from Products.PluginIndexes.DateRangeIndex.DateRangeIndex import DateRangeIndex @@ -31,6 +35,7 @@ class Dummy(object): def __init__(self, num): + self.id = str(num) self.num = num def big(self): @@ -74,7 +79,12 @@ class TestCatalogQueryKey(cleanup.CleanUp, unittest.TestCase): def setUp(self): cleanup.CleanUp.setUp(self) + self.zcat = self._build_zcatalog() + + def _build_zcatalog(self): + zcat = ZCatalog('catalog') + zcat.addColumn('id') zcat._catalog.addIndex('big', BooleanIndex('big')) zcat._catalog.addIndex('start', DateIndex('start')) zcat._catalog.addIndex('date', DateRangeIndex('date', 'start', 'end')) @@ -87,8 +97,8 @@ def setUp(self): obj = Dummy(i) zcat.catalog_object(obj, str(i)) - self.zcat = zcat - + return zcat + def _apply_query(self, query): cache = _get_cache() @@ -110,28 +120,28 @@ def _apply_query(self, query): rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) - def _get_cache_key(self, query=None): - catalog = self.zcat._catalog - query = catalog.make_query(query) - return CatalogCacheKey(catalog, query=query).key + def _get_cache_key(self, zcatalog, query): + catalog = zcatalog._catalog + q = catalog.make_query(query) + return CatalogCacheKey(catalog, query=q).key def test_make_key(self): query = {'big': True} expect = (('catalog',), frozenset([('big', (True,), self.length)])) - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) query = {'start': '2013-07-01'} expect = (('catalog',), frozenset([('start', ('2013-07-01',), self.length)])) - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} expect = (('catalog',), frozenset([('date', ('2013-07-05',), self.length), ('numbers', (1, 3), self.length), ('path', ('/1',), self.length)])) - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) queries = [{'big': True, 'b_start': 0}, {'big': True, 'b_start': 0, 'b_size': 5}, @@ -142,7 +152,7 @@ def test_make_key(self): expect = (('catalog',), frozenset([('big', (True,), self.length)])) for query in queries: - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) def test_cache(self): query = {'big': True} @@ -179,3 +189,91 @@ def test_cache_invalidate(self): rset1 = list(map(lambda x: x.getRID(), res1)) rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) + + def test_cache_mvcc_aware(self): + st = MinimalMemoryStorage() + db = DB(st) + + query = {'big': True} + cache = _get_cache() + cache.invalidateAll() + + # init catalog + cn = db.open() + r = cn.root() + r['zcat'] = self._build_zcatalog() + obj = Dummy(20) + r['zcat'].catalog_object(obj, obj.id) + transaction.get().commit() + + # 1st thread + tm1 = transaction.TransactionManager() + cn1 = db.open(transaction_manager=tm1) + + # 2nd thread + tm2 = transaction.TransactionManager() + cn2 = db.open(transaction_manager=tm2) + + r1 = cn1.root() + # catalog new object + obj = Dummy(21) + r1['zcat'].catalog_object(obj, obj.id) + + # dont cache if catalog has changed + self.assertEqual(self._get_cache_key(r1['zcat'], query), None) + tm1.get().commit() + + r2 = cn2.root() + # catalog new object + obj = Dummy(22) + r2['zcat'].catalog_object(obj, obj.id) + + # dont cache if catalog has changed + self.assertEqual(self._get_cache_key(r2['zcat'], query), None) + + res2 = r2['zcat'].search(query) + indexed_ids = {rec.id for rec in res2} + self.assertTrue(obj.id in indexed_ids) + + # raise conflict error because catalog was changed in tm1 + self.assertRaises(ConflictError, tm2.get().commit) + + tm2.get().abort() + + # try it again + r2 = cn2.root() + obj = Dummy(22) + r2['zcat'].catalog_object(obj, obj.id) + + res2 = r2['zcat'].search(query) + indexed_ids = {rec.id for rec in res2} + self.assertTrue(obj.id in indexed_ids) + + tm2.get().commit() + + cn1.sync() + # query without changing catalog + r1 = cn1.root() + qkey = self._get_cache_key(r1['zcat'], query) + + # query key is not None, results will be cached + self.assertEqual(qkey, (('catalog',), + frozenset({('big', (True,), 12)}))) + + res1 = r1['zcat'].search(query) + tm1.get().commit() + + r2 = cn2.root() + res2 = r2['zcat'].search(query) + tm2.get().commit() + + # compare result + rset1 = list(map(lambda x: x.getRID(), res1)) + rset2 = list(map(lambda x: x.getRID(), res2)) + self.assertEqual(rset1, rset2) + + stats = cache.getStatistics() + hits = stats[0]['hits'] + misses = stats[0]['misses'] + self.assertEqual((hits, misses), (1, 1)) + From 1be89a481793de1eb70f14771e6455f7a0fa3d20 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Fri, 20 Apr 2018 13:42:32 +0200 Subject: [PATCH 16/18] Implement transaction isolation syncer --- .../PluginIndexes/BooleanIndex/tests.py | 10 +- src/Products/PluginIndexes/DateIndex/tests.py | 10 +- .../PluginIndexes/DateRangeIndex/tests.py | 10 +- .../PluginIndexes/KeywordIndex/tests.py | 10 +- .../PluginIndexes/PathIndex/PathIndex.py | 6 +- src/Products/PluginIndexes/PathIndex/tests.py | 10 +- .../PluginIndexes/TopicIndex/TopicIndex.py | 6 +- .../PluginIndexes/TopicIndex/tests.py | 10 +- src/Products/PluginIndexes/UUIDIndex/tests.py | 10 +- .../PluginIndexes/tests/test_unindex.py | 12 +- src/Products/PluginIndexes/unindex.py | 6 +- src/Products/ZCatalog/cache.py | 264 ++++++++++++++++-- src/Products/ZCatalog/tests/test_cache.py | 150 ++++++---- 13 files changed, 382 insertions(+), 132 deletions(-) diff --git a/src/Products/PluginIndexes/BooleanIndex/tests.py b/src/Products/PluginIndexes/BooleanIndex/tests.py index 0a50834a..c5208190 100644 --- a/src/Products/PluginIndexes/BooleanIndex/tests.py +++ b/src/Products/PluginIndexes/BooleanIndex/tests.py @@ -257,19 +257,19 @@ def test_reindexation_when_index_reversed(self): def test_getCounter(self): index = self._makeOne() - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) obj = Dummy(1, True) index.index_object(obj.id, obj) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(obj.id) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear is a change index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) diff --git a/src/Products/PluginIndexes/DateIndex/tests.py b/src/Products/PluginIndexes/DateIndex/tests.py index 9c434fe3..274b8b88 100644 --- a/src/Products/PluginIndexes/DateIndex/tests.py +++ b/src/Products/PluginIndexes/DateIndex/tests.py @@ -337,21 +337,21 @@ def test_getCounter(self): from DateTime import DateTime index = self._makeOne() - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) index.index_object(1, Dummy('b', DateTime(0))) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(1) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear is a change index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) def test_precision(self): from DateTime import DateTime diff --git a/src/Products/PluginIndexes/DateRangeIndex/tests.py b/src/Products/PluginIndexes/DateRangeIndex/tests.py index 5f9cb75f..36fffc6e 100644 --- a/src/Products/PluginIndexes/DateRangeIndex/tests.py +++ b/src/Products/PluginIndexes/DateRangeIndex/tests.py @@ -299,22 +299,22 @@ def test_resultset(self): def test_getCounter(self): index = self._makeOne('work', 'start', 'stop') - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) k, obj = dummies[0] index.index_object(k, obj) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(k) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear is a change index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) def test_precision(self): precision = 5 diff --git a/src/Products/PluginIndexes/KeywordIndex/tests.py b/src/Products/PluginIndexes/KeywordIndex/tests.py index 898c2a8e..44216913 100644 --- a/src/Products/PluginIndexes/KeywordIndex/tests.py +++ b/src/Products/PluginIndexes/KeywordIndex/tests.py @@ -275,19 +275,19 @@ def test_value_removes(self): def test_getCounter(self): index = self._makeOne('foo') - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) obj = Dummy(['hello']) index.index_object(10, obj) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(10) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear is a change index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) diff --git a/src/Products/PluginIndexes/PathIndex/PathIndex.py b/src/Products/PluginIndexes/PathIndex/PathIndex.py index bce4b728..5ce20f66 100644 --- a/src/Products/PluginIndexes/PathIndex/PathIndex.py +++ b/src/Products/PluginIndexes/PathIndex/PathIndex.py @@ -200,11 +200,7 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - counter = self._counter is not None and self._counter() or 0 - changed = (self._counter is not None and - self._counter._p_changed or False) - - return (counter, changed) + return self._counter is not None and self._counter() or 0 def numObjects(self): """ See IPluggableIndex. diff --git a/src/Products/PluginIndexes/PathIndex/tests.py b/src/Products/PluginIndexes/PathIndex/tests.py index f6025417..75cb809c 100644 --- a/src/Products/PluginIndexes/PathIndex/tests.py +++ b/src/Products/PluginIndexes/PathIndex/tests.py @@ -537,19 +537,19 @@ def test__search_w_level_0(self): def test_getCounter(self): index = self._makeOne() - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) doc = Dummy('/aa/bb') index.index_object(1, doc) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(1) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) diff --git a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py index 80c91e83..54c1a431 100644 --- a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py +++ b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py @@ -103,11 +103,7 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - counter = self._counter is not None and self._counter() or 0 - changed = (self._counter is not None and - self._counter._p_changed or False) - - return (counter, changed) + return self._counter is not None and self._counter() or 0 def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/PluginIndexes/TopicIndex/tests.py b/src/Products/PluginIndexes/TopicIndex/tests.py index b2a7fcfb..a6dedf8b 100644 --- a/src/Products/PluginIndexes/TopicIndex/tests.py +++ b/src/Products/PluginIndexes/TopicIndex/tests.py @@ -97,18 +97,18 @@ def test_getCounter(self): index = self.TI index._counter.set(0) - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) index.index_object(1, Obj(1, 'doc1')) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(1) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) diff --git a/src/Products/PluginIndexes/UUIDIndex/tests.py b/src/Products/PluginIndexes/UUIDIndex/tests.py index c2089469..e7dee9c5 100644 --- a/src/Products/PluginIndexes/UUIDIndex/tests.py +++ b/src/Products/PluginIndexes/UUIDIndex/tests.py @@ -179,19 +179,19 @@ def test_non_unique(self): def test_getCounter(self): index = self._makeOne('foo') - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) obj = Dummy('a') index.index_object(10, obj) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(10) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear is a change index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) diff --git a/src/Products/PluginIndexes/tests/test_unindex.py b/src/Products/PluginIndexes/tests/test_unindex.py index 37acbc5a..66eeefef 100644 --- a/src/Products/PluginIndexes/tests/test_unindex.py +++ b/src/Products/PluginIndexes/tests/test_unindex.py @@ -27,7 +27,6 @@ def _getTargetClass(self): return UnIndex def _makeOne(self, *args, **kw): - index = self._getTargetClass()(*args, **kw) class DummyZCatalog(SimpleItem): @@ -145,10 +144,9 @@ def testQuery(record, expect=1): testQuery(record) def test_getCounter(self): - index = self._makeOne('counter') - self.assertEqual(index.getCounter(), (0, False)) + self.assertEqual(index.getCounter(), 0) class Dummy(object): id = 1 @@ -156,18 +154,18 @@ class Dummy(object): obj = Dummy() index.index_object(obj.id, obj) - self.assertEqual(index.getCounter(), (1, False)) + self.assertEqual(index.getCounter(), 1) index.unindex_object(obj.id) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), (2, False)) + self.assertEqual(index.getCounter(), 2) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), (3, False)) + self.assertEqual(index.getCounter(), 3) def test_no_type_error(self): ''' Check that on Python 3.6 we do not get a TypeError when trying diff --git a/src/Products/PluginIndexes/unindex.py b/src/Products/PluginIndexes/unindex.py index b6d5b761..20f00128 100644 --- a/src/Products/PluginIndexes/unindex.py +++ b/src/Products/PluginIndexes/unindex.py @@ -308,11 +308,7 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - counter = self._counter is not None and self._counter() or 0 - changed = (self._counter is not None and - self._counter._p_changed or False) - - return (counter, changed) + return self._counter is not None and self._counter() or 0 def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index f44988b4..d63bd464 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -10,25 +10,45 @@ # FOR A PARTICULAR PURPOSE # ############################################################################## +import logging +import transaction from Acquisition import aq_base from Acquisition import aq_parent from DateTime import DateTime -from plone.memoize.volatile import DontCache from Products.PluginIndexes.interfaces import IIndexCounter from Products.PluginIndexes.interfaces import IDateRangeIndex from Products.PluginIndexes.interfaces import IDateIndex + from plone.memoize import ram +from ZODB.utils import p64, z64 +from ZODB.POSException import ReadConflictError +from six.moves._thread import allocate_lock +from six.moves._thread import _local + +LOG = logging.getLogger('Zope.ZCatalog.cache') +_marker = (z64, z64) + + +class DontCache(Exception): + pass -class CatalogCacheKey(object): + +class CatalogQueryKey(object): def __init__(self, catalog, query=None): self.catalog = catalog self.cid = self.get_id() self.query = query - self.key = self.make_key(query) + + def __call__(self): + return self.key + + @property + def key(self): + return self.make_key(self.query) def get_id(self): catalog = self.catalog @@ -43,7 +63,7 @@ def get_id(self): def make_key(self, query): if query is None: - return None + raise DontCache catalog = self.catalog @@ -63,22 +83,27 @@ def skip(name, value): if name in catalog.indexes: index = catalog.getIndex(name) if IIndexCounter.providedBy(index): - counter, changed = index.getCounter() - - # cannot cache if index was updated - if changed: - return None + if ( + not index._p_jar or catalog._p_jar._storage is not + index._p_jar._storage + ): + # paranoid check; if the catalog and the indexes + # are not managed in the same storage, no + # transaction aware cache key can be generated. + raise DontCache + + counter = index.getCounter() else: # cache key invalidation cannot be supported if - # any index of query cannot be tested for changes - return None + # any index of query cannot be tested for updates + raise DontCache elif skip(name, value): # applying the query to indexes is invariant of # sort or pagination options continue else: - # return None if query has a nonexistent index key - return None + # raise DontCache if query has a nonexistent index key + raise DontCache if isinstance(value, dict): kvl = [] @@ -93,8 +118,8 @@ def skip(name, value): keys.append((name, value, counter)) key = frozenset(keys) - cache_key = (self.cid, key) - return cache_key + query_key = (self.cid, key) + return query_key def _convert_datum(self, index, value): @@ -129,19 +154,203 @@ def convert_datetime(dt): return res -# plone memoize cache key -def _apply_query_plan_cachekey(method, catalog, plan, query): - cc = CatalogCacheKey(catalog, query) - if cc.key is None: - raise DontCache - return cc.key +class QueryCacheManager(object): + def __init__(self, cache=dict()): + self.cache = cache + + def __call__(self, func): + + def decorated(catalog, plan, query): + try: + query_key = CatalogQueryKey(catalog, query).key + except DontCache: + return func(catalog, plan, query) + + try: + # check high-water mark of catalog's zodb connection + zodb_storage = catalog._p_jar._storage + zodb_storage._start + except AttributeError: + return func(catalog, plan, query) + + key = '{0}.{1}:{2}'.format(func.__module__, + func.__name__, query_key) + + # convert key to 64 bit hash (not really required) + oid = p64(hash(key) & ((1 << 64) - 1)) + + tca = TransactionalCacheAdapter(zodb_storage, self.cache) + + try: + value = tca[oid] + # HIT + except KeyError: + value = func(catalog, plan, query) + tca[oid] = value + # MISS & SET + + tca.registerAfterCommitHook() + + return value + + return decorated + + +class TransactionalCacheAdapter(object): + """ """ + lock = allocate_lock() + thread_local = _local() + + def __init__(self, ref_storage, cache): + + self.cache_adapter = cache + + # get thread isolated local buffer/cache + buffer_id = '_v_{0}_buffer'.format(self.__class__.__name__) + try: + self._cache = getattr(self.thread_local, buffer_id) + except AttributeError: + setattr(self.thread_local, buffer_id, {}) + self._cache = getattr(self.thread_local, buffer_id) + + # commit buffer + self._uncommitted = {} + + self._ref_storage = ref_storage + self._start = ref_storage._start + self._tid = z64 + self._registered = False + + def __getitem__(self, oid): + """ """ + try: + # try to get value from local or shared cache + (_oid, serial, value) = self._load(oid) + except KeyError: + raise KeyError(oid) + + return value + + def __setitem__(self, oid, value): + """ """ + self._cache[oid] = self._uncommitted[oid] = (oid, z64, value) + + def _load(self, oid): + """ """ + if oid not in self._cache: + try: + (_oid, serial) = self.cache_adapter[oid] -def cache(fun): - @ram.cache(_apply_query_plan_cachekey) - def decorator(*args, **kwargs): - return fun(*args, **kwargs) - return decorator + if _oid != z64 and _oid != oid: + # should never happen + raise ValueError + + if serial > self._start: + # we have a non-current revision of cache entry + raise ReadConflictError + + ckey = str(hash((_oid, serial))) + value = self.cache_adapter[ckey] + self._cache[oid] = (oid, serial, value) + + # cache HIT + return self._cache[oid] + + except KeyError: + # cache MISS + raise + + (_oid, serial, value) = self._cache[oid] + + if serial > self._start: + # we have a non-current revision of cache entry + raise ReadConflictError + + return self._cache[oid] + + def _prepare(self): + """ + """ + + for oid in self._uncommitted: + (_oid, serial) = self.cache_adapter.get( + oid, default=_marker) + + if _oid != z64 and oid != _oid: + # should never happen + raise ValueError + + if serial > self._start: + # should never happen; we have a non-current + # revision of cache entry + LOG.debug('{0}._prepare: non-current revision' + ' of cache entry detected'.format( + self.__class__.__name__)) + + raise ReadConflictError + + # get lastTransaction of catalog's zodb connection + self._tid = self._ref_storage.lastTransaction() + + def _commit(self): + """ + """ + + # store uncommited values into shared cache + for oid in self._uncommitted: + (_oid, serial, value) = self._uncommitted[oid] + + self.cache_adapter[oid] = (oid, self._tid) + + ckey = str(hash((oid, self._tid))) + self.cache_adapter[ckey] = value + + self._cleanup() + + def _abort(self): + """ + """ + self._cleanup() + + def _cleanup(self): + """ + """ + self._uncommitted.clear() + self._cache.clear() + + def commit_hook(self, success, args=()): + + # if commit failed -> return + if not success: + self._abort() + return + + # no new data to commit -> return + if not self._uncommitted: + self._cleanup() + return + + try: + with self.lock: + self._prepare() + self._commit() + except Exception: + self._abort() + LOG.debug('Store of cache value failed') + return + + def registerAfterCommitHook(self): + """ + """ + if not self._registered: + transaction.get().addAfterCommitHook(self.commit_hook) + self._registered = True + + +# plone memoize marker +def _apply_query_plan_cachekey(): + pass # Make sure we provide test isolation, works only for ramcache @@ -160,3 +369,6 @@ def _cache_clear(): from zope.testing.cleanup import addCleanUp # NOQA addCleanUp(_cache_clear) del addCleanUp + +# cache decorator for catalog._apply_query_plan +cache = QueryCacheManager(cache=ram.store_in_cache(_apply_query_plan_cachekey)) diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 48ca5ec9..dd7d3c45 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -14,10 +14,11 @@ import unittest import transaction -from zope.testing import cleanup -from ZODB.tests.test_storage import MinimalMemoryStorage from ZODB import DB from ZODB.POSException import ConflictError +from zope.testing import cleanup +from ZODB.tests.test_storage import MinimalMemoryStorage +from ZODB.utils import newTid from Products.PluginIndexes.BooleanIndex.BooleanIndex import BooleanIndex from Products.PluginIndexes.DateRangeIndex.DateRangeIndex import DateRangeIndex @@ -28,10 +29,20 @@ from Products.PluginIndexes.UUIDIndex.UUIDIndex import UUIDIndex from Products.ZCatalog.Catalog import Catalog from Products.ZCatalog.ZCatalog import ZCatalog -from Products.ZCatalog.cache import CatalogCacheKey +from Products.ZCatalog.cache import CatalogQueryKey from Products.ZCatalog.cache import _get_cache +class DummyMVCCAdapter(object): + def __init__(self): + + class DummyStorage(object): + def __init__(self): + self._start = newTid(None) + + self._storage = DummyStorage() + + class Dummy(object): def __init__(self, num): @@ -54,7 +65,7 @@ def end(self): return '2013-07-%.2d' % (self.num + 2) -class TestCatalogCacheKey(unittest.TestCase): +class TestCatalogQueryKey(unittest.TestCase): def setUp(self): self.cat = Catalog('catalog') @@ -62,7 +73,9 @@ def setUp(self): def _makeOne(self, catalog=None, query=None): if catalog is None: catalog = self.cat - return CatalogCacheKey(catalog, query=query) + catalog._p_jar = DummyMVCCAdapter() + + return CatalogQueryKey(catalog, query=query) def test_get_id(self): cache_key = self._makeOne() @@ -75,14 +88,26 @@ def test_get_id_persistent(self): self.assertEquals(cache_key.get_id(), ('catalog', )) -class TestCatalogQueryKey(cleanup.CleanUp, unittest.TestCase): +class TestCatalogQueryCaching(cleanup.CleanUp, unittest.TestCase): def setUp(self): cleanup.CleanUp.setUp(self) - self.zcat = self._build_zcatalog() - + + st = MinimalMemoryStorage() + db = DB(st) + cache = _get_cache() + cache.invalidateAll() + + # init catalog + cn = db.open() + r = cn.root() + + r['zcat'] = self._build_zcatalog() + self.zcat = r['zcat'] + transaction.get().commit() + def _build_zcatalog(self): - + zcat = ZCatalog('catalog') zcat.addColumn('id') zcat._catalog.addIndex('big', BooleanIndex('big')) @@ -98,21 +123,29 @@ def _build_zcatalog(self): zcat.catalog_object(obj, str(i)) return zcat - + def _apply_query(self, query): cache = _get_cache() + transaction.get().commit() + + # 1st search MISS res1 = self.zcat.search(query) - stats = cache.getStatistics() + transaction.get().commit() + stats = cache.getStatistics() + hits = stats[0]['hits'] misses = stats[0]['misses'] + # 2nd search HIT res2 = self.zcat.search(query) - stats = cache.getStatistics() + transaction.get().commit() + stats = cache.getStatistics() + # check if chache hits - self.assertEqual(stats[0]['hits'], hits + 1) + self.assertEqual(stats[0]['hits'], hits + 2) self.assertEqual(stats[0]['misses'], misses) # compare result @@ -123,25 +156,29 @@ def _apply_query(self, query): def _get_cache_key(self, zcatalog, query): catalog = zcatalog._catalog q = catalog.make_query(query) - return CatalogCacheKey(catalog, query=q).key + return CatalogQueryKey(catalog, query=q).key def test_make_key(self): query = {'big': True} expect = (('catalog',), frozenset([('big', (True,), self.length)])) - self.assertEquals(self._get_cache_key(self.zcat, query), expect) + qkey = self._get_cache_key(self.zcat, query) + self.assertEqual(qkey, expect) query = {'start': '2013-07-01'} expect = (('catalog',), - frozenset([('start', ('2013-07-01',), self.length)])) - self.assertEquals(self._get_cache_key(self.zcat, query), expect) + frozenset([('start', ('2013-07-01',), + self.length)])) + qkey = self._get_cache_key(self.zcat, query) + self.assertEqual(qkey, expect) query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} expect = (('catalog',), frozenset([('date', ('2013-07-05',), self.length), ('numbers', (1, 3), self.length), ('path', ('/1',), self.length)])) - self.assertEquals(self._get_cache_key(self.zcat, query), expect) + qkey = self._get_cache_key(self.zcat, query) + self.assertEqual(qkey, expect) queries = [{'big': True, 'b_start': 0}, {'big': True, 'b_start': 0, 'b_size': 5}, @@ -151,10 +188,13 @@ def test_make_key(self): ] expect = (('catalog',), frozenset([('big', (True,), self.length)])) + for query in queries: - self.assertEquals(self._get_cache_key(self.zcat, query), expect) + qkey = self._get_cache_key(self.zcat, query) + self.assertEqual(qkey, expect) def test_cache(self): + query = {'big': True} self._apply_query(query) @@ -166,11 +206,16 @@ def test_cache(self): def test_cache_invalidate(self): cache = _get_cache() + cache.invalidateAll() + transaction.get().commit() + query = {'big': False} res1 = self.zcat.search(query) - stats = cache.getStatistics() + transaction.get().commit() + stats = cache.getStatistics() + hits = stats[0]['hits'] misses = stats[0]['misses'] @@ -179,62 +224,63 @@ def test_cache_invalidate(self): self.zcat.catalog_object(obj, str(20)) res2 = self.zcat.search(query) - stats = cache.getStatistics() + transaction.get().commit() - # check if chache misses + stats = cache.getStatistics() + + # check if cache misses (__getitem__ + commit) self.assertEqual(stats[0]['hits'], hits) - self.assertEqual(stats[0]['misses'], misses + 1) + self.assertEqual(stats[0]['misses'], misses + 2) # compare result rset1 = list(map(lambda x: x.getRID(), res1)) rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) - def test_cache_mvcc_aware(self): + def test_cache_mvcc_concurrent_writes(self): st = MinimalMemoryStorage() db = DB(st) query = {'big': True} cache = _get_cache() cache.invalidateAll() - - # init catalog - cn = db.open() - r = cn.root() - r['zcat'] = self._build_zcatalog() - obj = Dummy(20) - r['zcat'].catalog_object(obj, obj.id) - transaction.get().commit() # 1st thread tm1 = transaction.TransactionManager() cn1 = db.open(transaction_manager=tm1) + # init catalog + r1 = cn1.root() + r1['zcat'] = self._build_zcatalog() + obj = Dummy(20) + + r1['zcat'].catalog_object(obj, obj.id) + tm1.get().commit() + cn1.sync() + # 2nd thread tm2 = transaction.TransactionManager() cn2 = db.open(transaction_manager=tm2) - + r1 = cn1.root() # catalog new object obj = Dummy(21) r1['zcat'].catalog_object(obj, obj.id) # dont cache if catalog has changed - self.assertEqual(self._get_cache_key(r1['zcat'], query), None) tm1.get().commit() - + stats = cache.getStatistics() + self.assertEqual(stats, tuple()) + r2 = cn2.root() # catalog new object obj = Dummy(22) r2['zcat'].catalog_object(obj, obj.id) - # dont cache if catalog has changed - self.assertEqual(self._get_cache_key(r2['zcat'], query), None) - res2 = r2['zcat'].search(query) indexed_ids = {rec.id for rec in res2} self.assertTrue(obj.id in indexed_ids) - + # raise conflict error because catalog was changed in tm1 self.assertRaises(ConflictError, tm2.get().commit) @@ -244,11 +290,11 @@ def test_cache_mvcc_aware(self): r2 = cn2.root() obj = Dummy(22) r2['zcat'].catalog_object(obj, obj.id) - + res2 = r2['zcat'].search(query) indexed_ids = {rec.id for rec in res2} self.assertTrue(obj.id in indexed_ids) - + tm2.get().commit() cn1.sync() @@ -257,23 +303,29 @@ def test_cache_mvcc_aware(self): qkey = self._get_cache_key(r1['zcat'], query) # query key is not None, results will be cached - self.assertEqual(qkey, (('catalog',), - frozenset({('big', (True,), 12)}))) + expect = ((('catalog',), frozenset( + {('big', (True,), 12)}))) + self.assertEqual(qkey, expect) - res1 = r1['zcat'].search(query) - tm1.get().commit() + # cache store + res1 = r1['zcat'].search(query) + transaction.get().commit() + r2 = cn2.root() - res2 = r2['zcat'].search(query) - tm2.get().commit() + # cache hit + res2 = r2['zcat'].search(query) + transaction.get().commit() + # compare result rset1 = list(map(lambda x: x.getRID(), res1)) rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) + cache = _get_cache() stats = cache.getStatistics() + hits = stats[0]['hits'] misses = stats[0]['misses'] - self.assertEqual((hits, misses), (1, 1)) - + self.assertEqual((hits, misses), (2, 4)) From caa2dd623181584228221183f3232bb3af735852 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Mon, 9 Jul 2018 18:15:23 +0200 Subject: [PATCH 17/18] Use volatile attribute of instance for buffering cache values --- src/Products/ZCatalog/cache.py | 36 ++++++++--------- src/Products/ZCatalog/tests/test_cache.py | 49 ++++++++++++++++++----- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index d63bd464..441a780b 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -25,8 +25,8 @@ from ZODB.utils import p64, z64 from ZODB.POSException import ReadConflictError +from functools import wraps from six.moves._thread import allocate_lock -from six.moves._thread import _local LOG = logging.getLogger('Zope.ZCatalog.cache') _marker = (z64, z64) @@ -67,6 +67,13 @@ def make_key(self, query): catalog = self.catalog + # check high-water mark of catalog's zodb connection + try: + zodb_storage = catalog._p_jar._storage + zodb_storage._start + except AttributeError: + raise DontCache + def skip(name, value): if name in ['b_start', 'b_size']: return True @@ -84,7 +91,7 @@ def skip(name, value): index = catalog.getIndex(name) if IIndexCounter.providedBy(index): if ( - not index._p_jar or catalog._p_jar._storage is not + not index._p_jar or zodb_storage is not index._p_jar._storage ): # paranoid check; if the catalog and the indexes @@ -160,26 +167,20 @@ def __init__(self, cache=dict()): def __call__(self, func): + @wraps(func) def decorated(catalog, plan, query): try: query_key = CatalogQueryKey(catalog, query).key except DontCache: return func(catalog, plan, query) - try: - # check high-water mark of catalog's zodb connection - zodb_storage = catalog._p_jar._storage - zodb_storage._start - except AttributeError: - return func(catalog, plan, query) - key = '{0}.{1}:{2}'.format(func.__module__, func.__name__, query_key) # convert key to 64 bit hash (not really required) oid = p64(hash(key) & ((1 << 64) - 1)) - tca = TransactionalCacheAdapter(zodb_storage, self.cache) + tca = TransactionalCacheAdapter(catalog, self.cache) try: value = tca[oid] @@ -199,25 +200,24 @@ def decorated(catalog, plan, query): class TransactionalCacheAdapter(object): """ """ lock = allocate_lock() - thread_local = _local() - def __init__(self, ref_storage, cache): + def __init__(self, instance, cache): self.cache_adapter = cache # get thread isolated local buffer/cache buffer_id = '_v_{0}_buffer'.format(self.__class__.__name__) try: - self._cache = getattr(self.thread_local, buffer_id) + self._cache = getattr(instance, buffer_id) except AttributeError: - setattr(self.thread_local, buffer_id, {}) - self._cache = getattr(self.thread_local, buffer_id) + setattr(instance, buffer_id, {}) + self._cache = getattr(instance, buffer_id) # commit buffer self._uncommitted = {} - self._ref_storage = ref_storage - self._start = ref_storage._start + self._zodb_storage = instance._p_jar._storage + self._start = self._zodb_storage._start self._tid = z64 self._registered = False @@ -291,7 +291,7 @@ def _prepare(self): raise ReadConflictError # get lastTransaction of catalog's zodb connection - self._tid = self._ref_storage.lastTransaction() + self._tid = self._zodb_storage.lastTransaction() def _commit(self): """ diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index dd7d3c45..de6a99e6 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -215,7 +215,7 @@ def test_cache_invalidate(self): transaction.get().commit() stats = cache.getStatistics() - + hits = stats[0]['hits'] misses = stats[0]['misses'] @@ -227,7 +227,7 @@ def test_cache_invalidate(self): transaction.get().commit() stats = cache.getStatistics() - + # check if cache misses (__getitem__ + commit) self.assertEqual(stats[0]['hits'], hits) self.assertEqual(stats[0]['misses'], misses + 2) @@ -238,6 +238,9 @@ def test_cache_invalidate(self): self.assertEqual(rset1, rset2) def test_cache_mvcc_concurrent_writes(self): + # remember: a search result corresponds to two key + # value pairs in the cache + st = MinimalMemoryStorage() db = DB(st) @@ -278,10 +281,12 @@ def test_cache_mvcc_concurrent_writes(self): r2['zcat'].catalog_object(obj, obj.id) res2 = r2['zcat'].search(query) + # cache miss indexed_ids = {rec.id for rec in res2} self.assertTrue(obj.id in indexed_ids) # raise conflict error because catalog was changed in tm1 + # don't prepare set for after commit self.assertRaises(ConflictError, tm2.get().commit) tm2.get().abort() @@ -291,7 +296,9 @@ def test_cache_mvcc_concurrent_writes(self): obj = Dummy(22) r2['zcat'].catalog_object(obj, obj.id) + # cache miss and prepare set for after commit res2 = r2['zcat'].search(query) + indexed_ids = {rec.id for rec in res2} self.assertTrue(obj.id in indexed_ids) @@ -307,25 +314,49 @@ def test_cache_mvcc_concurrent_writes(self): {('big', (True,), 12)}))) self.assertEqual(qkey, expect) - # cache store - + # cache miss and prepare set for after commit res1 = r1['zcat'].search(query) + + # cache store transaction.get().commit() - + + stats = cache.getStatistics() + hits = stats[0]['hits'] + misses = stats[0]['misses'] + self.assertEqual((hits, misses), (0, 5)) + r2 = cn2.root() - # cache hit + # cache hit res2 = r2['zcat'].search(query) transaction.get().commit() - + # compare result rset1 = list(map(lambda x: x.getRID(), res1)) rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) - cache = _get_cache() stats = cache.getStatistics() hits = stats[0]['hits'] misses = stats[0]['misses'] - self.assertEqual((hits, misses), (2, 4)) + self.assertEqual((hits, misses), (2, 5)) + + # check usage instance cache + r2 = cn2.root() + # cache hit + res21 = r2['zcat'].search(query) + # instance cache hit + res22 = r2['zcat'].search(query) + + # compare result + rset21 = list(map(lambda x: x.getRID(), res21)) + rset22 = list(map(lambda x: x.getRID(), res22)) + self.assertEqual(rset21, rset22) + + stats = cache.getStatistics() + hits = stats[0]['hits'] + misses = stats[0]['misses'] + self.assertEqual((hits, misses), (4, 5)) + + transaction.get().commit() From ed38d23b83e4ec61b2f67d378453f9e2cce86467 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Wed, 17 Apr 2019 17:48:24 +0200 Subject: [PATCH 18/18] Fix flake8 --- src/Products/ZCatalog/tests/test_cache.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index de6a99e6..43138695 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -56,13 +56,13 @@ def numbers(self): return (self.num, self.num + 1) def getPhysicalPath(self): - return '/%s' % self.num + return '/{0}'.format(self.num) def start(self): - return '2013-07-%.2d' % (self.num + 1) + return '2013-07-{0:0=2d}'.format(self.num + 1) def end(self): - return '2013-07-%.2d' % (self.num + 2) + return '2013-07-{0:0=2d}'.format(self.num + 1) class TestCatalogQueryKey(unittest.TestCase): @@ -79,13 +79,13 @@ def _makeOne(self, catalog=None, query=None): def test_get_id(self): cache_key = self._makeOne() - self.assertEquals(cache_key.get_id(), - ('', 'NonPersistentCatalog')) + self.assertEqual(cache_key.get_id(), + ('', 'NonPersistentCatalog')) def test_get_id_persistent(self): zcat = ZCatalog('catalog') cache_key = self._makeOne(zcat._catalog) - self.assertEquals(cache_key.get_id(), ('catalog', )) + self.assertEqual(cache_key.get_id(), ('catalog', )) class TestCatalogQueryCaching(cleanup.CleanUp, unittest.TestCase): @@ -134,7 +134,7 @@ def _apply_query(self, query): transaction.get().commit() stats = cache.getStatistics() - + hits = stats[0]['hits'] misses = stats[0]['misses'] @@ -143,7 +143,7 @@ def _apply_query(self, query): transaction.get().commit() stats = cache.getStatistics() - + # check if chache hits self.assertEqual(stats[0]['hits'], hits + 2) self.assertEqual(stats[0]['misses'], misses)