Properly test both INVALIDATE and UPDATE strategy. Fix bug with incorrect size calculations on updating existing elements.

5 files changed, 100 insertions, 66 deletions

diff --git a/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp b/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp
index 80471235d1b..7e94f2d01f7 100644
--- a/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp
+++ b/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp
@@ -440,7 +440,8 @@ makeDoc(const DocumentTypeRepo &repo, uint32_t i, bool extra_field)
 
 class VisitCacheStore {
 public:
-    VisitCacheStore();
+    using UpdateStrategy=DocumentStore::Config::UpdateStrategy;
+    VisitCacheStore(UpdateStrategy strategy);
     ~VisitCacheStore();
     IDocumentStore & getStore() { return _datastore; }
     void write(uint32_t id) {
@@ -510,10 +511,12 @@ VisitCacheStore::VerifyVisitor::~VerifyVisitor() {
     EXPECT_EQUAL(_expected.size(), _actual.size());
 }
 
-VisitCacheStore::VisitCacheStore() :
+
+VisitCacheStore::VisitCacheStore(UpdateStrategy strategy) :
     _myDir("visitcache"),
     _repo(makeDocTypeRepoConfig()),
-    _config(DocumentStore::Config(CompressionConfig::LZ4, 1000000, 0).allowVisitCaching(true),
+    _config(DocumentStore::Config(CompressionConfig::LZ4, 1000000, 0)
+                    .allowVisitCaching(true).updateStrategy(strategy),
             LogDataStore::Config().setMaxFileSize(50000).setMaxBucketSpread(3.0)
                     .setFileConfig(WriteableFileChunk::Config(CompressionConfig(), 16384))),
     _fileHeaderContext(),
@@ -535,8 +538,58 @@ verifyCacheStats(CacheStats cs, size_t hits, size_t misses, size_t elements, siz
     EXPECT_GREATER_EQUAL(memory_used+20,  cs.memory_used);
 }
 
+TEST("test the update cache strategy") {
+    VisitCacheStore vcs(DocumentStore::Config::UpdateStrategy::UPDATE);
+    IDocumentStore & ds = vcs.getStore();
+    for (size_t i(1); i <= 10; i++) {
+        vcs.write(i);
+    }
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 0, 0, 0));
+    vcs.verifyRead(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 1, 1, 221));
+    vcs.write(8);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 1, 1, 221));
+    vcs.write(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 1, 1, 221));
+    vcs.verifyRead(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 1, 1, 1, 221));
+    vcs.remove(8);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 1, 1, 1, 221));
+    vcs.remove(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 1, 1, 0, 0));
+    vcs.write(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 1, 1, 0, 0));
+    vcs.verifyRead(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 1, 2, 1, 221));
+}
+
+TEST("test the invalidate cache strategy") {
+    VisitCacheStore vcs(DocumentStore::Config::UpdateStrategy::INVALIDATE);
+    IDocumentStore & ds = vcs.getStore();
+    for (size_t i(1); i <= 10; i++) {
+        vcs.write(i);
+    }
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 0, 0, 0));
+    vcs.verifyRead(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 1, 1, 221));
+    vcs.write(8);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 1, 1, 221));
+    vcs.write(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 1, 0, 0));
+    vcs.verifyRead(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 2, 1, 221));
+    vcs.remove(8);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 2, 1, 221));
+    vcs.remove(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 2, 0, 0));
+    vcs.write(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 2, 0, 0));
+    vcs.verifyRead(7);
+    TEST_DO(verifyCacheStats(ds.getCacheStats(), 0, 3, 1, 221));
+}
+
 TEST("test that the integrated visit cache works.") {
-    VisitCacheStore vcs;
+    VisitCacheStore vcs(DocumentStore::Config::UpdateStrategy::INVALIDATE);
     IDocumentStore & ds = vcs.getStore();
     for (size_t i(1); i <= 100; i++) {
         vcs.write(i);
diff --git a/searchlib/src/vespa/searchlib/docstore/documentstore.cpp b/searchlib/src/vespa/searchlib/docstore/documentstore.cpp
index a6c5f567454..6e784bf8c30 100644
--- a/searchlib/src/vespa/searchlib/docstore/documentstore.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/documentstore.cpp
@@ -194,7 +194,6 @@ BackingStore::read(DocumentIdT key, Value &value) const {
 void
 BackingStore::write(DocumentIdT lid, const Value & value)
 {
-    assert(value.getCompression() == _compression.type);
     vespalib::DataBuffer buf = value.decompressed();
     _backingStore.write(value.getSyncToken(), lid, buf.getData(), buf.getDataLen());
 }
@@ -304,12 +303,17 @@ DocumentStore::write(uint64_t syncToken, DocumentIdT lid, const vespalib::nbostr
                 _backingStore.write(syncToken, lid, stream.peek(), stream.size());
                 _cache->invalidate(lid);
                 break;
-            case Config::UpdateStrategy::UPDATE: {
-                Value value(syncToken);
-                value.set(vespalib::DataBuffer(), stream.size(), _store->getCompression());
-                _cache->write(lid, std::move(value));
+            case Config::UpdateStrategy::UPDATE:
+                if (_cache->hasKey(lid)) {
+                    Value value(syncToken);
+                    vespalib::DataBuffer buf(stream.size());
+                    buf.writeBytes(stream.peek(), stream.size());
+                    value.set(std::move(buf), stream.size(), _store->getCompression());
+                    _cache->write(lid, std::move(value));
+                } else {
+                    _backingStore.write(syncToken, lid, stream.peek(), stream.size());
+                }
                 break;
-            }
         }
         _visitCache->invalidate(lid); // The cost and complexity of this updating this is not worth it.
     } else {
@@ -503,31 +507,22 @@ WrapVisitor(Visitor &visitor,
 
 
 void
-DocumentStore::accept(IDocumentStoreReadVisitor &visitor,
-                      IDocumentStoreVisitorProgress &visitorProgress,
+DocumentStore::accept(IDocumentStoreReadVisitor &visitor, IDocumentStoreVisitorProgress &visitorProgress,
                       const DocumentTypeRepo &repo)
 {
-    WrapVisitor<IDocumentStoreReadVisitor> wrap(visitor, repo,
-                                                _store->getCompression(),
-                                                *this,
-                                                _backingStore.
-                                                tentativeLastSyncToken());
+    WrapVisitor<IDocumentStoreReadVisitor> wrap(visitor, repo, _store->getCompression(), *this,
+                                                _backingStore.tentativeLastSyncToken());
     WrapVisitorProgress wrapVisitorProgress(visitorProgress);
     _backingStore.accept(wrap, wrapVisitorProgress, false);
 }
 
 
 void
-DocumentStore::accept(IDocumentStoreRewriteVisitor &visitor,
-                      IDocumentStoreVisitorProgress &visitorProgress,
+DocumentStore::accept(IDocumentStoreRewriteVisitor &visitor, IDocumentStoreVisitorProgress &visitorProgress,
                       const DocumentTypeRepo &repo)
 {
-    WrapVisitor<IDocumentStoreRewriteVisitor> wrap(visitor,
-                                                   repo,
-                                                   _store->getCompression(),
-                                                   *this,
-                                                   _backingStore.
-                                                   tentativeLastSyncToken());
+    WrapVisitor<IDocumentStoreRewriteVisitor> wrap(visitor, repo, _store->getCompression(), *this,
+                                                   _backingStore.tentativeLastSyncToken());
     WrapVisitorProgress wrapVisitorProgress(visitorProgress);
     _backingStore.accept(wrap, wrapVisitorProgress, true);
 }
diff --git a/staging_vespalib/src/tests/stllike/cache_test.cpp b/staging_vespalib/src/tests/stllike/cache_test.cpp
index 142c4065673..6e852644602 100644
--- a/staging_vespalib/src/tests/stllike/cache_test.cpp
+++ b/staging_vespalib/src/tests/stllike/cache_test.cpp
@@ -28,36 +28,10 @@ public:
     }
 };
 
-class Test : public TestApp
-{
-public:
-    int Main() override;
-private:
-    typedef LruParam<uint32_t, string> P;
-    typedef Map<uint32_t, string> B;
-    void testCache();
-    void testCacheSize();
-    void testCacheSizeDeep();
-    void testCacheEntriesHonoured();
-    void testCacheMaxSizeHonoured();
-    void testThatMultipleRemoveOnOverflowIsFine();
-};
-
-int
-Test::Main()
-{
-    TEST_INIT("cache_test");
-    testCache();
-    testCacheSize();
-    testCacheSizeDeep();
-    testCacheEntriesHonoured();
-    testCacheMaxSizeHonoured();
-    testThatMultipleRemoveOnOverflowIsFine();
-    TEST_DONE();
-}
+using P = LruParam<uint32_t, string>;
+using B = Map<uint32_t, string>;
 
-void Test::testCache()
-{
+TEST("testCache") {
     B m;
     cache< CacheParam<P, B> > cache(m, -1);
     // Verfify start conditions.
@@ -74,24 +48,29 @@ void Test::testCache()
     EXPECT_TRUE(cache.size() == 1);
 }
 
-void Test::testCacheSize()
+TEST("testCacheSize")
 {
     B m;
     cache< CacheParam<P, B> > cache(m, -1);
     cache.write(1, "10 bytes string");
     EXPECT_EQUAL(80u, cache.sizeBytes());
+    cache.write(1, "10 bytes string"); // Still the same size
+    EXPECT_EQUAL(80u, cache.sizeBytes());
 }
 
-void Test::testCacheSizeDeep()
+TEST("testCacheSizeDeep")
 {
     B m;
     cache< CacheParam<P, B, zero<uint32_t>, size<string> > > cache(m, -1);
     cache.write(1, "15 bytes string");
     EXPECT_EQUAL(95u, cache.sizeBytes());
+    cache.write(1, "10 bytes s");
+    EXPECT_EQUAL(90u, cache.sizeBytes());
+    cache.write(1, "20 bytes string ssss");
+    EXPECT_EQUAL(100u, cache.sizeBytes());
 }
 
-void Test::testCacheEntriesHonoured()
-{
+TEST("testCacheEntriesHonoured") {
     B m;
     cache< CacheParam<P, B, zero<uint32_t>, size<string> > > cache(m, -1);
     cache.maxElements(1);
@@ -105,8 +84,7 @@ void Test::testCacheEntriesHonoured()
     EXPECT_EQUAL(96u, cache.sizeBytes());
 }
 
-void Test::testCacheMaxSizeHonoured()
-{
+TEST("testCacheMaxSizeHonoured") {
     B m;
     cache< CacheParam<P, B, zero<uint32_t>, size<string> > > cache(m, 200);
     cache.write(1, "15 bytes string");
@@ -123,8 +101,7 @@ void Test::testCacheMaxSizeHonoured()
     EXPECT_EQUAL(291u, cache.sizeBytes());
 }
 
-void Test::testThatMultipleRemoveOnOverflowIsFine()
-{
+TEST("testThatMultipleRemoveOnOverflowIsFine") {
     B m;
     cache< CacheParam<P, B, zero<uint32_t>, size<string> > > cache(m, 2000);
     
@@ -159,5 +136,4 @@ void Test::testThatMultipleRemoveOnOverflowIsFine()
     EXPECT_EQUAL(2924u, cache.sizeBytes());
 }
 
-
-TEST_APPHOOK(Test)
+TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/staging_vespalib/src/vespa/vespalib/stllike/cache.h b/staging_vespalib/src/vespa/vespalib/stllike/cache.h
index 6b3cec659aa..99b601f7c3c 100644
--- a/staging_vespalib/src/vespa/vespalib/stllike/cache.h
+++ b/staging_vespalib/src/vespa/vespalib/stllike/cache.h
@@ -148,6 +148,7 @@ private:
     mutable size_t      _race;
     mutable size_t      _insert;
     mutable size_t      _write;
+    mutable size_t      _update;
     mutable size_t      _erase;
     mutable size_t      _invalidate;
     mutable size_t      _lookup;
diff --git a/staging_vespalib/src/vespa/vespalib/stllike/cache.hpp b/staging_vespalib/src/vespa/vespalib/stllike/cache.hpp
index 1ba168fa77b..906621d623c 100644
--- a/staging_vespalib/src/vespa/vespalib/stllike/cache.hpp
+++ b/staging_vespalib/src/vespa/vespalib/stllike/cache.hpp
@@ -61,6 +61,7 @@ cache<P>::cache(BackingStore & b, size_t maxBytes) :
     _race(0),
     _insert(0),
     _write(0),
+    _update(0),
     _erase(0),
     _invalidate(0),
     _lookup(0),
@@ -122,13 +123,21 @@ template< typename P >
 void
 cache<P>::write(const K & key, V value)
 {
+    size_t newSize = calcSize(key, value);
     vespalib::LockGuard storeGuard(getLock(key));
+    {
+        vespalib::LockGuard guard(_hashLock);
+        if (Lru::hasKey(key)) {
+            _sizeBytes -= calcSize(key, (*this)[key]);
+            _update++;
+        }
+    }
+
     _store.write(key, value);
     {
         vespalib::LockGuard guard(_hashLock);
-        size_t sz = calcSize(key, value);
         (*this)[key] = std::move(value);
-        _sizeBytes += sz;
+        _sizeBytes += newSize;
         _write++;
     }
 }