From 88110c95be596ccd5b331271002e19ffce19e575 Mon Sep 17 00:00:00 2001
From: Christopher Haster <geky@geky.net>
Date: Tue, 20 Feb 2024 00:52:04 -0600
Subject: [PATCH] Attempted to better reuse lfsr_bd_readnext in lfsr_bd_read

lfsr_bd_readnext and lfsr_bd_read are almost the same function, with the
significant exception of cache-bypassing reads.

Bypassing reads are an interesting optimization in littlefs. Since we're
dealing with very constrained amounts of RAM, it's not uncommon for read
calls to have more RAM available than our internal caches. In this case
bypassing the cache 1. avoids copies, 2. reduces bus transaction, and 3.
leaves data in the rcache which may be useful for ongoing smaller
queries.

But bypassing reads make no sense for lfsr_bd_readnext, since
lfsr_bd_readnext calls have no buffer by definition.

This leads to a bit of a mess when you try to make lfsr_bd_read call
lfsr_bd_readnext, bypassing reads are lfsr_bd_read specific, but we need
to check for rcache/pcache prioritization first, which is the same in
both lfsr_bd_read and lfsr_bd_readnext.

The solution here is to duplicate the rcache/pcache prioritization
checks as a precondition for bypassing reads, at least deduplicating the
actual rcache/pcache memcpy. This isn't the greatest because memcpy is
actually pretty cheap in terms of code cost. But I don't see a better
organization.

The result is less code savings than expected.

Unfortunately this also comes with a high stack cost, just because of
the additional read->readnext stack frame. lfsr_bd_read is usually the
leaf on the hot path stack-wise, making the worst-case stack quite
sensitive to any changes to this function:

                      code          stack
  before readnext:   33584           2792
  dup read/readnext: 33804 (+0.7%)   2808 (+0.7%)
  rec read/readnext: 33744 (+0.5%)   2872 (+2.9%)
---
 lfs.c | 70 ++++++++++++++++-------------------------------------------
 1 file changed, 19 insertions(+), 51 deletions(-)

diff --git a/lfs.c b/lfs.c
index 05e5785b..3492364a 100644
--- a/lfs.c
+++ b/lfs.c
@@ -237,42 +237,18 @@ static int lfsr_bd_read(lfs_t *lfs,
     uint8_t *buffer_ = buffer;
     lfs_size_t size_ = size;
     while (size_ > 0) {
-        // already in pcache?
-        if (block == lfs->pcache.block
-                && off_ < lfs->pcache.off + lfs->pcache.size
-                && off_ >= lfs->pcache.off) {
-            lfs_size_t d = lfs_min(
-                    size_,
-                    lfs->pcache.size - (off_-lfs->pcache.off));
-            memcpy(buffer_, &lfs->pcache.buffer[off_-lfs->pcache.off], d);
-
-            off_ += d;
-            hint_ -= d;
-            buffer_ += d;
-            size_ -= d;
-            continue;
-        }
-
-        // already in rcache?
-        if (block == lfs->rcache.block
-                && off_ < lfs->rcache.off + lfs->rcache.size
-                && off_ >= lfs->rcache.off) {
-            lfs_size_t d = lfs_min(
-                    size_,
-                    lfs->rcache.size - (off_-lfs->rcache.off));
-            memcpy(buffer_, &lfs->rcache.buffer[off_-lfs->rcache.off], d);
-
-            off_ += d;
-            hint_ -= d;
-            buffer_ += d;
-            size_ -= d;
-            continue;
-        }
-
         // bypass cache?
         if (size_ >= hint_
                 && off_ % lfs->cfg->read_size == 0
-                && size_ >= lfs->cfg->read_size) {
+                && size_ >= lfs->cfg->read_size
+                // pcache takes priority
+                && !(block == lfs->pcache.block
+                    && off_ < lfs->pcache.off + lfs->pcache.size
+                    && off_ >= lfs->pcache.off)
+                // rcache takes priority
+                && !(block == lfs->rcache.block
+                    && off_ < lfs->rcache.off + lfs->rcache.size
+                    && off_ >= lfs->rcache.off)) {
             lfs_size_t d = lfs_aligndown(size_, lfs->cfg->read_size);
             int err = lfsr_bd_read_(lfs, block, off_, buffer_, d);
             if (err) {
@@ -286,28 +262,20 @@ static int lfsr_bd_read(lfs_t *lfs,
             continue;
         }
 
-        // drop rcache in case read fails
-        lfsr_cache_drop(&lfs->rcache);
-
-        // load to cache, first condition can no longer fail
-        lfs_size_t off__ = lfs_aligndown(off_, lfs->cfg->read_size);
-        // watch out for overflow when hint_=-1
-        lfs_size_t size__ = lfs_alignup(
-                (off_-off__) + lfs_min(
-                    lfs_max(size_, hint_),
-                    lfs_min(
-                        lfs->cfg->cache_size - (off_-off__),
-                        lfs->cfg->block_size - off_)),
-                lfs->cfg->read_size);
-        int err = lfsr_bd_read_(lfs, block, off__,
-                lfs->rcache.buffer, size__);
+        const uint8_t *buffer__;
+        lfs_size_t size__;
+        int err = lfsr_bd_readnext(lfs, block, off_, hint_, size_,
+                &buffer__, &size__);
         if (err) {
             return err;
         }
 
-        lfs->rcache.block = block;
-        lfs->rcache.off = off__;
-        lfs->rcache.size = size__;
+        memcpy(buffer_, buffer__, size__);
+
+        off_ += size__;
+        hint_ -= size__;
+        buffer_ += size__;
+        size_ -= size__;
     }
 
     return 0;