From 8316fbdfd15d383ad8c8db2451f1a2ebaa649365 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 23 May 2025 19:00:27 -0500 Subject: [PATCH] Rerouted lfsr_file_read_'s leaf eviction -> lfsr_file_crystallize This commit actually does two things: 1. Opportunistically marks caches as flushed if they were included in the crystallization region in lfsr_file_crystallize 2. Reroutes lfsr_file_read_ through lfsr_file_crystallize to minimize stack cost --- Digging into why lazy-crystallization adds so much stack, it seems the main reason is because lfsr_file_read_ drags in lfsr_file_flush, which puts the entirety of the stack hot-path under both lfsr_file_read and lfsr_file_read_. But why are we calling lfsr_file_flush? And not just lfsr_file_crystallize to claim the leaf? Isn't the cache flushed in lfsr_file_read before reading? The one concerning case is when reads bypass the cache (read > cache_size). With cache-bypassing reads, it's entirely possible for lfsr_file_read_ to end up with unflushed data. lfsr_file_read's logic gives the cache priority in this case, so it's not like we're going to read outdated data or anything, but if we crystallize without flushing we risk wasting erased-state that will need to be recrystallized later. What's extra humorous is our crystallization logic _does_ correctly write out the cache, it just doesn't clear the LFS_o_UNFLUSH bit because it doesn't know if progress has been made. So to avoid this, all we need to do is add an explicit check to lfsr_file_crystallize that clears the LFS_o_UNFLUSH bit if our cache ends up written out as a part of crystallization. Note this is slightly more powerful than lfsr_file_flush, since we don't _need_ to flush the cache if it's not in our crystallization region. As an extra plus this affects all lfsr_file_crystallize calls, so now lfsr_file_truncate/fruncate also avoid unnecessary recrystallization. That's some good code reuse right there! --- Long story short, rerouting lfsr_file_read_ through lfsr_file_crystallize moves it off the stack hot-path, bringing our stack down to almost pre-lazy-crystallization levels: code stack ctx before: 37140 2304 636 after: 37184 (+0.1%) 2288 (-0.7%) 636 (+0.0%) At a code cost, but this code also allows lfsr_file_read/truncate/ fruncate to avoid recrystallization with opportunistic flushes in cases where we need to discard file->leaf. --- lfs.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/lfs.c b/lfs.c index 6b9feab1..71139937 100644 --- a/lfs.c +++ b/lfs.c @@ -11724,15 +11724,21 @@ static int lfsr_file_lookup(lfs_t *lfs, const lfsr_file_t *file, return 0; } +// needed in lfsr_file_read_ +static int lfsr_file_crystallize(lfs_t *lfs, lfsr_file_t *file); + static lfs_ssize_t lfsr_file_read_(lfs_t *lfs, lfsr_file_t *file, lfs_off_t pos, uint8_t *buffer, lfs_size_t size) { // need to fetch a new leaf? if (!(pos >= file->leaf.pos && pos < file->leaf.pos + file->leaf.weight)) { - // leaf in use? we need to flush it + // leaf in use? we need to crystallize/graft it + // + // it would be easier to just call lfsr_file_flush here, but + // we don't want to drag in the extra stack usage if (lfsr_o_isungraft(file->b.o.flags) || lfsr_o_isuncryst(file->b.o.flags)) { - int err = lfsr_file_flush(lfs, file); + int err = lfsr_file_crystallize(lfs, file); if (err) { return err; } @@ -12398,6 +12404,10 @@ static int lfsr_file_crystallize_(lfs_t *lfs, lfsr_file_t *file, } static int lfsr_file_crystallize(lfs_t *lfs, lfsr_file_t *file) { + bool flushable = ( + file->cache.pos + >= file->leaf.pos + lfsr_bptr_size(&file->leaf.bptr)); + // finish crystallizing if (lfsr_o_isuncryst(file->b.o.flags)) { // uncrystallized files must be unsynced @@ -12432,6 +12442,13 @@ static int lfsr_file_crystallize(lfs_t *lfs, lfsr_file_t *file) { file->b.o.flags &= ~LFS_o_UNGRAFT; } + // eagerly mark as flushed if this included all of our cache + if (flushable + && file->leaf.pos + lfsr_bptr_size(&file->leaf.bptr) + >= file->cache.pos + file->cache.size) { + file->b.o.flags &= ~LFS_o_UNFLUSH; + } + return 0; }