From 13c7c9996598ae1a304ad36a3e08c973c8d25fa0 Mon Sep 17 00:00:00 2001 From: John Preston Date: Sun, 19 Aug 2018 18:49:45 +0300 Subject: [PATCH] Ignore put queries that don't change anything. --- .../storage/cache/storage_cache_database.cpp | 93 ++++++++++++------- .../cache/storage_cache_database_tests.cpp | 48 +++++++++- 2 files changed, 107 insertions(+), 34 deletions(-) diff --git a/Telegram/SourceFiles/storage/cache/storage_cache_database.cpp b/Telegram/SourceFiles/storage/cache/storage_cache_database.cpp index a64a208931..e7762a66a7 100644 --- a/Telegram/SourceFiles/storage/cache/storage_cache_database.cpp +++ b/Telegram/SourceFiles/storage/cache/storage_cache_database.cpp @@ -279,6 +279,7 @@ private: EncryptionKey &key); bool readHeader(); bool writeHeader(); + void readBinlog(); size_type readBinlogRecords(bytes::const_span data); size_type readBinlogRecordSize(bytes::const_span data) const; @@ -316,6 +317,8 @@ private: void setMapEntry(const Key &key, Entry &&entry); void eraseMapEntry(const Map::const_iterator &i); + void recordEntryAccess(const Key &key); + QByteArray readValueData(PlaceId place, size_type size) const; Version findAvailableVersion() const; QString versionPath() const; @@ -324,13 +327,17 @@ private: QString placePath(PlaceId place) const; bool isFreePlace(PlaceId place) const; + template - QString writeKeyPlaceGeneric( + base::optional writeKeyPlaceGeneric( StoreRecord &&record, const Key &key, - size_type size, + const QByteArray &value, + uint32 checksum); + base::optional writeKeyPlace( + const Key &key, + const QByteArray &value, uint32 checksum); - QString writeKeyPlace(const Key &key, size_type size, uint32 checksum); void writeMultiRemoveLazy(); void writeMultiRemove(); void writeMultiAccessLazy(); @@ -953,11 +960,17 @@ void Database::put( _removing.erase(key); const auto checksum = CountChecksum(bytes::make_span(value)); - const auto path = writeKeyPlace(key, value.size(), checksum); - if (path.isEmpty()) { + const auto maybepath = writeKeyPlace(key, value, checksum); + if (!maybepath) { invokeCallback(done, ioError(binlogPath())); return; + } else if (maybepath->isEmpty()) { + // Nothing changed. + invokeCallback(done, Error::NoError()); + recordEntryAccess(key); + return; } + const auto path = *maybepath; File data; const auto result = data.open(path, File::Mode::Write, _key); switch (result) { @@ -987,18 +1000,26 @@ void Database::put( } template -QString Database::writeKeyPlaceGeneric( +base::optional Database::writeKeyPlaceGeneric( StoreRecord &&record, const Key &key, - size_type size, + const QByteArray &value, uint32 checksum) { - Expects(size <= _settings.maxDataSize); + Expects(value.size() <= _settings.maxDataSize); + const auto size = size_type(value.size()); record.key = key; record.size = ReadTo(size); record.checksum = checksum; if (const auto i = _map.find(key); i != end(_map)) { - record.place = i->second.place; + const auto &already = i->second; + if (already.tag == record.tag + && already.size == size + && already.checksum == checksum + && readValueData(already.place, size) == value) { + return QString(); + } + record.place = already.place; } else { do { bytes::set_random(bytes::object_as_span(&record.place)); @@ -1015,12 +1036,12 @@ QString Database::writeKeyPlaceGeneric( return result; } -QString Database::writeKeyPlace( +base::optional Database::writeKeyPlace( const Key &key, - size_type size, + const QByteArray &data, uint32 checksum) { if (!_settings.trackEstimatedTime) { - return writeKeyPlaceGeneric(Store(), key, size, checksum); + return writeKeyPlaceGeneric(Store(), key, data, checksum); } auto record = StoreWithTime(); record.time = countTimePoint(); @@ -1031,7 +1052,7 @@ QString Database::writeKeyPlace( record.time.system = _latestSystemTime; record.time.relativeAdvancement = 0; } - return writeKeyPlaceGeneric(std::move(record), key, size, checksum); + return writeKeyPlaceGeneric(std::move(record), key, data, checksum); } void Database::get(const Key &key, FnMut done) { @@ -1046,38 +1067,46 @@ void Database::get(const Key &key, FnMut done) { } const auto &entry = i->second; - const auto path = placePath(entry.place); + auto result = readValueData(entry.place, entry.size); + if (result.isEmpty()) { + invokeCallback(done, QByteArray()); + } else if (CountChecksum(bytes::make_span(result)) != entry.checksum) { + invokeCallback(done, QByteArray()); + } else { + invokeCallback(done, std::move(result)); + recordEntryAccess(key); + } +} + +QByteArray Database::readValueData(PlaceId place, size_type size) const { + const auto path = placePath(place); File data; const auto result = data.open(path, File::Mode::Read, _key); switch (result) { case File::Result::Failed: - invokeCallback(done, QByteArray()); - break; - - case File::Result::WrongKey: - invokeCallback(done, QByteArray()); - break; - + case File::Result::WrongKey: return QByteArray(); case File::Result::Success: { - auto result = QByteArray(entry.size, Qt::Uninitialized); + auto result = QByteArray(size, Qt::Uninitialized); const auto bytes = bytes::make_span(result); const auto read = data.readWithPadding(bytes); - if (read != entry.size || CountChecksum(bytes) != entry.checksum) { - invokeCallback(done, QByteArray()); - } else { - invokeCallback(done, std::move(result)); - if (_settings.trackEstimatedTime) { - _accessed.emplace(key); - writeMultiAccessLazy(); - } - startDelayedPruning(); + if (read != size) { + return QByteArray(); } + return result; } break; - default: Unexpected("Result in Database::get."); } } +void Database::recordEntryAccess(const Key &key) { + if (!_settings.trackEstimatedTime) { + return; + } + _accessed.emplace(key); + writeMultiAccessLazy(); + startDelayedPruning(); +} + void Database::remove(const Key &key, FnMut done) { const auto i = _map.find(key); if (i != _map.end()) { diff --git a/Telegram/SourceFiles/storage/cache/storage_cache_database_tests.cpp b/Telegram/SourceFiles/storage/cache/storage_cache_database_tests.cpp index a4d36bb968..61f2a834ca 100644 --- a/Telegram/SourceFiles/storage/cache/storage_cache_database_tests.cpp +++ b/Telegram/SourceFiles/storage/cache/storage_cache_database_tests.cpp @@ -74,6 +74,7 @@ const auto GetValue = [](QByteArray value) { const auto Settings = [] { auto result = Database::Settings(); + result.trackEstimatedTime = false; result.writeBundleDelay = 1 * crl::time_type(1000); result.pruneTimeout = 1 * crl::time_type(1500); result.maxDataSize = 20; @@ -151,6 +152,42 @@ TEST_CASE("encrypted cache db", "[storage_cache_database]") { Semaphore.acquire(); REQUIRE((Value == TestValue2)); + db.close([&] { Semaphore.release(); }); + Semaphore.acquire(); + } + SECTION("overwriting values") { + Database db(name, Settings); + + db.open(key, GetResult); + Semaphore.acquire(); + REQUIRE(Result.type == Error::Type::None); + + const auto path = GetBinlogPath(); + + db.get(Key{ 0, 1 }, GetValue); + Semaphore.acquire(); + REQUIRE((Value == TestValue1)); + + const auto size = QFile(path).size(); + + db.put(Key{ 0, 1 }, TestValue2, GetResult); + Semaphore.acquire(); + REQUIRE(Result.type == Error::Type::None); + + const auto next = QFile(path).size(); + REQUIRE(next > size); + + db.get(Key{ 0, 1 }, GetValue); + Semaphore.acquire(); + REQUIRE((Value == TestValue2)); + + db.put(Key{ 0, 1 }, TestValue2, GetResult); + Semaphore.acquire(); + REQUIRE(Result.type == Error::Type::None); + + const auto same = QFile(path).size(); + REQUIRE(same == next); + db.close([&] { Semaphore.release(); }); Semaphore.acquire(); } @@ -206,7 +243,9 @@ TEST_CASE("cache db remove", "[storage_cache_database]") { TEST_CASE("cache db bundled actions", "[storage_cache_database]") { SECTION("db touched written lazily") { - Database db(name, Settings); + auto settings = Settings; + settings.trackEstimatedTime = true; + Database db(name, settings); db.clear(GetResult); Semaphore.acquire(); @@ -240,7 +279,9 @@ TEST_CASE("cache db bundled actions", "[storage_cache_database]") { Semaphore.acquire(); } SECTION("db touched written on close") { - Database db(name, Settings); + auto settings = Settings; + settings.trackEstimatedTime = true; + Database db(name, settings); db.clear(GetResult); Semaphore.acquire(); @@ -340,6 +381,7 @@ TEST_CASE("cache db bundled actions", "[storage_cache_database]") { TEST_CASE("cache db limits", "[storage_cache_database]") { SECTION("db both limit") { auto settings = Settings; + settings.trackEstimatedTime = true; settings.totalSizeLimit = 17 * 3 + 1; settings.totalTimeLimit = 4; Database db(name, settings); @@ -376,6 +418,7 @@ TEST_CASE("cache db limits", "[storage_cache_database]") { } SECTION("db size limit") { auto settings = Settings; + settings.trackEstimatedTime = true; settings.totalSizeLimit = 17 * 3 + 1; Database db(name, settings); @@ -442,6 +485,7 @@ TEST_CASE("cache db limits", "[storage_cache_database]") { } SECTION("db time limit") { auto settings = Settings; + settings.trackEstimatedTime = true; settings.totalTimeLimit = 3; Database db(name, settings);