From bf01525fc154594d3b5eb7f57bfd83c9daf1359d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 12 May 2017 12:18:08 +0800 Subject: [PATCH] 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 --- components/nvs_flash/src/nvs_page.cpp | 21 ----------- components/nvs_flash/src/nvs_page.hpp | 35 ------------------- .../nvs_flash/test_nvs_host/test_nvs.cpp | 24 +++++++++++-- 3 files changed, 21 insertions(+), 59 deletions(-) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index a24c7214b..d764b4350 100644 --- 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(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 66b6e847c..b0b3de3ba 100644 --- 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 { @@ -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 ff35a84d1..2877852ae 100644 --- 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 #include +#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);