nvs: remove search cache at page level

Since read cache was introduced at page level, search cache became
useless in terms of reducing the number of flash read operations.
In addition to that, search cache used an assumption that if pointers to
keys are identical, the keys are also identical, which was proven wrong
by applications which generate key names dynamically.

This change removes CachedFindInfo, and all its uses. This is done at
expense of a small extra number of CPU operations (looking up a value in
the read cache is slightly more expensive) but no extra flash read
operations.

Ref TW12505
Ref https://github.com/espressif/arduino-esp32/issues/365

diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp
index a24c7214bb1559da6fcd3f5302928704990c8dc3..d764b435051327db54ec58ab976987e1edf34bdf 100644 (file)
--- a/components/nvs_flash/src/nvs_page.cpp
+++ b/components/nvs_flash/src/nvs_page.cpp
@@ -296,9 +296,6 @@ esp_err_t Page::eraseItem(uint8_t nsIndex, ItemType datatype, const char* key)
     if (rc != ESP_OK) {
         return rc;
     }
-    if (CachedFindInfo(nsIndex, datatype, key) == mFindInfo) {
-        invalidateCache();
-    }
     return eraseEntryAndSpan(index);
 }
 
@@ -386,10 +383,6 @@ esp_err_t Page::moveItem(Page& other)
         return ESP_ERR_NVS_NOT_FOUND;
     }
 
-    if (mFindInfo.itemIndex() == mFirstUsedEntry) {
-        invalidateCache();
-    }
-
     if (other.mState == PageState::UNINITIALIZED) {
         auto err = other.initialize();
         if (err != ESP_OK) {
@@ -608,7 +601,6 @@ esp_err_t Page::initialize()
 
     mNextFreeEntry = 0;
     std::fill_n(mEntryTable.data(), mEntryTable.byteSize() / sizeof(uint32_t), 0xffffffff);
-    invalidateCache();
     return ESP_OK;
 }
 
@@ -685,11 +677,6 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
         return ESP_ERR_NVS_NOT_FOUND;
     }
 
-    CachedFindInfo findInfo(nsIndex, datatype, key);
-    if (mFindInfo == findInfo) {
-        findBeginIndex = mFindInfo.itemIndex();
-    }
-
     size_t start = mFirstUsedEntry;
     if (findBeginIndex > mFirstUsedEntry && findBeginIndex < ENTRY_COUNT) {
         start = findBeginIndex;
@@ -745,8 +732,6 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
         }
 
         itemIndex = i;
-        findInfo.setItemIndex(static_cast<uint32_t>(itemIndex));
-        mFindInfo = findInfo;
 
         return ESP_OK;
     }
@@ -805,12 +790,6 @@ esp_err_t Page::markFull()
     }
     return alterPageState(PageState::FULL);
 }
-
-
-void Page::invalidateCache()
-{
-    mFindInfo = CachedFindInfo();
-}
     
 const char* Page::pageStateToName(PageState ps)
 {

diff --git a/components/nvs_flash/src/nvs_page.hpp b/components/nvs_flash/src/nvs_page.hpp
index 66b6e847c8bbab75fe23c276fb6f23012411f71b..b0b3de3ba9dd59540bad7c59163560860829d00b 100644 (file)
--- a/components/nvs_flash/src/nvs_page.hpp
+++ b/components/nvs_flash/src/nvs_page.hpp
@@ -28,38 +28,6 @@
 namespace nvs
 {
 
-class CachedFindInfo
-{
-public:
-    CachedFindInfo() { }
-    CachedFindInfo(uint8_t nsIndex, ItemType type, const char* key) :
-        mKeyPtr(key),
-        mNsIndex(nsIndex),
-        mType(type)
-    {
-    }
-
-    bool operator==(const CachedFindInfo& other) const
-    {
-        return mKeyPtr != nullptr && mKeyPtr == other.mKeyPtr && mType == other.mType && mNsIndex == other.mNsIndex;
-    }
-
-    void setItemIndex(uint32_t index)
-    {
-        mItemIndex = index;
-    }
-    uint32_t itemIndex() const
-    {
-        return mItemIndex;
-    }
-
-protected:
-    uint32_t mItemIndex = 0;
-    const char* mKeyPtr = nullptr;
-    uint8_t mNsIndex = 0;
-    ItemType mType;
-
-};
 
 class Page : public intrusive_list_node<Page>
 {
@@ -161,8 +129,6 @@ public:
 
     esp_err_t erase();
 
-    void invalidateCache();
-
     void debugDump() const;
 
 protected:
@@ -235,7 +201,6 @@ protected:
     uint16_t mUsedEntryCount = 0;
     uint16_t mErasedEntryCount = 0;
 
-    CachedFindInfo mFindInfo;
     HashList mHashList;
 
     static const uint32_t HEADER_OFFSET = 0;

diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp
index ff35a84d110f2d99c5f97154a074455df9cc16a7..2877852aee5beb4a34e324185c12875186567e58 100644 (file)
--- a/components/nvs_flash/test_nvs_host/test_nvs.cpp
+++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp
@@ -18,6 +18,9 @@
 #include <sstream>
 #include <iostream>
 
+#define TEST_ESP_ERR(rc, res) CHECK((rc) == (res))
+#define TEST_ESP_OK(rc) CHECK((rc) == ESP_OK)
+
 using namespace std;
 using namespace nvs;
 
@@ -209,6 +212,24 @@ TEST_CASE("can write and read variable length data", "[nvs]")
     CHECK(memcmp(buf, str, strlen(str)) == 0);
 }
 
+TEST_CASE("different key names are distinguished even if the pointer is the same", "[nvs]")
+{
+    SpiFlashEmulator emu(1);
+    Page page;
+    TEST_ESP_OK(page.load(0));
+    TEST_ESP_OK(page.writeItem(1, "i1", 1));
+    TEST_ESP_OK(page.writeItem(1, "i2", 2));
+    int32_t value;
+    char keyname[10] = {0};
+    for (int i = 0; i < 2; ++i) {
+        strncpy(keyname, "i1", sizeof(keyname) - 1);
+        TEST_ESP_OK(page.readItem(1, keyname, value));
+        CHECK(value == 1);
+        strncpy(keyname, "i2", sizeof(keyname) - 1);
+        TEST_ESP_OK(page.readItem(1, keyname, value));
+        CHECK(value == 2);
+    }
+}
 
 TEST_CASE("can init PageManager in empty flash", "[nvs]")
 {
@@ -428,9 +449,6 @@ TEST_CASE("can erase items", "[nvs]")
     CHECK(storage.readItem(3, "key00222", val) == ESP_ERR_NVS_NOT_FOUND);
 }
 
-#define TEST_ESP_ERR(rc, res) CHECK((rc) == (res))
-#define TEST_ESP_OK(rc) CHECK((rc) == ESP_OK)
-
 TEST_CASE("nvs api tests", "[nvs]")
 {
     SpiFlashEmulator emu(10);