Skip to content

Commit 238dbc7

Browse files
gekygeky
committed
Abandoned data-backed cache, use indirect lfs3_data_t on stack
This abandons the data-backed cache idea due to concerns around readability and maintainability. Mixing const/mutable buffers in lfs3_data_t was not great. Instead, we now just allocate an indirect lfs3_data_t on the stack in lfs3_file_sync_ to avoid the previous undefined behavior. This actually results in less stack usage total, due to lfs3_file_t allocations in lfs3_set/read, and avoid the more long-term memory cost in lfs3_file_t: code stack ctx before: 36832 2376 684 after: 36840 (+0.0%) 2368 (-0.3%) 684 (+0.0%) Oh. And lfs3_file_sync_ isn't even on the stack hot-path, so this is a net benefit over the previous cache -> data cast: code stack ctx before sa: 36844 2368 684 after sa: 36840 (-0.0%) 2368 (+0.0%) 684 (+0.0%) Still less cool though.
1 parent 5035aa5 commit 238dbc7

File tree

2 files changed

+60
-54
lines changed

2 files changed

+60
-54
lines changed

‎lfs3.c‎

Lines changed: 56 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -11586,7 +11586,7 @@ static inline void lfs3_file_discardcache(lfs3_file_t *file) {
1158611586
#ifndef LFS3_KVONLY
1158711587
file->cache.pos = 0;
1158811588
#endif
11589-
file->cache.d.size = 0;
11589+
file->cache.size = 0;
1159011590
}
1159111591

1159211592
#ifndef LFS3_KVONLY
@@ -11611,7 +11611,7 @@ static inline lfs3_size_t lfs3_file_cachesize(lfs3_t *lfs3,
1161111611

1161211612
static inline lfs3_off_t lfs3_file_size_(const lfs3_file_t *file) {
1161311613
return lfs3_max(
11614-
LFS3_IFDEF_KVONLY(0, file->cache.pos) + file->cache.d.size,
11614+
LFS3_IFDEF_KVONLY(0, file->cache.pos) + file->cache.size,
1161511615
file->b.shrub.r.weight);
1161611616
}
1161711617

@@ -11729,17 +11729,17 @@ int lfs3_file_opencfg_(lfs3_t *lfs3, lfs3_file_t *file,
1172911729
// the file cache, so make sure not to clobber it
1173011730
if (lfs3_o_iswrset(file->b.h.flags)) {
1173111731
file->b.h.flags |= LFS3_o_UNFLUSH;
11732-
file->cache.d.u.buffer_ = file->cfg->cache_buffer;
11732+
file->cache.buffer = file->cfg->cache_buffer;
1173311733
#ifndef LFS3_KVONLY
1173411734
file->cache.pos = 0;
1173511735
#endif
11736-
file->cache.d.size = file->cfg->cache_size;
11736+
file->cache.size = file->cfg->cache_size;
1173711737
} else if (file->cfg->cache_buffer) {
11738-
file->cache.d.u.buffer_ = file->cfg->cache_buffer;
11738+
file->cache.buffer = file->cfg->cache_buffer;
1173911739
} else {
1174011740
#ifndef LFS3_KVONLY
11741-
file->cache.d.u.buffer_ = lfs3_malloc(lfs3_file_cachesize(lfs3, file));
11742-
if (!file->cache.d.u.buffer_) {
11741+
file->cache.buffer = lfs3_malloc(lfs3_file_cachesize(lfs3, file));
11742+
if (!file->cache.buffer) {
1174311743
return LFS3_ERR_NOMEM;
1174411744
}
1174511745
#else
@@ -11821,9 +11821,9 @@ int lfs3_file_opencfg_(lfs3_t *lfs3, lfs3_file_t *file,
1182111821
// small file wrset? can we atomically commit everything in one
1182211822
// commit? currently this is only possible via lfs3_set
1182311823
if (lfs3_o_iswrset(file->b.h.flags)
11824-
&& file->cache.d.size <= lfs3->cfg->inline_size
11825-
&& file->cache.d.size <= lfs3->cfg->fragment_size
11826-
&& file->cache.d.size < lfs3->cfg->crystal_thresh) {
11824+
&& file->cache.size <= lfs3->cfg->inline_size
11825+
&& file->cache.size <= lfs3->cfg->fragment_size
11826+
&& file->cache.size < lfs3->cfg->crystal_thresh) {
1182711827
// we need to mark as unsync for sync to do anything
1182811828
file->b.h.flags |= LFS3_o_UNSYNC;
1182911829

@@ -11944,7 +11944,7 @@ static void lfs3_file_close_(lfs3_t *lfs3, const lfs3_file_t *file) {
1194411944
(void)lfs3;
1194511945
// clean up memory
1194611946
if (!file->cfg->cache_buffer) {
11947-
lfs3_free(file->cache.d.u.buffer_);
11947+
lfs3_free(file->cache.buffer);
1194811948
}
1194911949

1195011950
// are we orphaning a file?
@@ -12165,14 +12165,14 @@ lfs3_ssize_t lfs3_file_read(lfs3_t *lfs3, lfs3_file_t *file,
1216512165
lfs3_ssize_t d = lfs3_min(size, lfs3_file_size_(file) - pos_);
1216612166

1216712167
// any data in our cache?
12168-
if (pos_ < file->cache.pos + file->cache.d.size
12169-
&& file->cache.d.size != 0) {
12168+
if (pos_ < file->cache.pos + file->cache.size
12169+
&& file->cache.size != 0) {
1217012170
if (pos_ >= file->cache.pos) {
1217112171
lfs3_ssize_t d_ = lfs3_min(
1217212172
d,
12173-
file->cache.d.size - (pos_ - file->cache.pos));
12173+
file->cache.size - (pos_ - file->cache.pos));
1217412174
lfs3_memcpy(buffer_,
12175-
&file->cache.d.u.buffer_[pos_ - file->cache.pos],
12175+
&file->cache.buffer[pos_ - file->cache.pos],
1217612176
d_);
1217712177

1217812178
pos_ += d_;
@@ -12207,13 +12207,13 @@ lfs3_ssize_t lfs3_file_read(lfs3_t *lfs3, lfs3_file_t *file,
1220712207
// try to fill our cache with some data
1220812208
if (!lfs3_o_isunflush(file->b.h.flags)) {
1220912209
lfs3_ssize_t d_ = lfs3_file_readnext(lfs3, file,
12210-
pos_, file->cache.d.u.buffer_, d);
12210+
pos_, file->cache.buffer, d);
1221112211
if (d_ < 0) {
1221212212
LFS3_ASSERT(d != LFS3_ERR_NOENT);
1221312213
return d_;
1221412214
}
1221512215
file->cache.pos = pos_;
12216-
file->cache.d.size = d_;
12216+
file->cache.size = d_;
1221712217
continue;
1221812218
}
1221912219
}
@@ -13338,10 +13338,10 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
1333813338
// note we need to clear the cache anyways to avoid any
1333913339
// out-of-date data
1334013340
file->cache.pos = pos + size - lfs3_file_cachesize(lfs3, file);
13341-
lfs3_memcpy(file->cache.d.u.buffer_,
13341+
lfs3_memcpy(file->cache.buffer,
1334213342
&buffer_[size - lfs3_file_cachesize(lfs3, file)],
1334313343
lfs3_file_cachesize(lfs3, file));
13344-
file->cache.d.size = lfs3_file_cachesize(lfs3, file);
13344+
file->cache.size = lfs3_file_cachesize(lfs3, file);
1334513345

1334613346
file->b.h.flags &= ~LFS3_o_UNFLUSH;
1334713347
written += size;
@@ -13363,25 +13363,25 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
1336313363
//
1336413364
if (!lfs3_o_isunflush(file->b.h.flags)
1336513365
|| (pos >= file->cache.pos
13366-
&& pos <= file->cache.pos + file->cache.d.size
13366+
&& pos <= file->cache.pos + file->cache.size
1336713367
&& pos
1336813368
< file->cache.pos
1336913369
+ lfs3_file_cachesize(lfs3, file))) {
1337013370
// unused cache? we can move it where we need it
1337113371
if (!lfs3_o_isunflush(file->b.h.flags)) {
1337213372
file->cache.pos = pos;
13373-
file->cache.d.size = 0;
13373+
file->cache.size = 0;
1337413374
}
1337513375

1337613376
lfs3_size_t d = lfs3_min(
1337713377
size,
1337813378
lfs3_file_cachesize(lfs3, file)
1337913379
- (pos - file->cache.pos));
13380-
lfs3_memcpy(&file->cache.d.u.buffer_[pos - file->cache.pos],
13380+
lfs3_memcpy(&file->cache.buffer[pos - file->cache.pos],
1338113381
buffer_,
1338213382
d);
13383-
file->cache.d.size = lfs3_max(
13384-
file->cache.d.size,
13383+
file->cache.size = lfs3_max(
13384+
file->cache.size,
1338513385
pos+d - file->cache.pos);
1338613386

1338713387
file->b.h.flags |= LFS3_o_UNFLUSH;
@@ -13394,7 +13394,7 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
1339413394

1339513395
// flush our cache so the above can't fail
1339613396
err = lfs3_file_flush_(lfs3, file,
13397-
file->cache.pos, file->cache.d.u.buffer_, file->cache.d.size);
13397+
file->cache.pos, file->cache.buffer, file->cache.size);
1339813398
if (err) {
1339913399
goto failed;
1340013400
}
@@ -13458,13 +13458,13 @@ int lfs3_file_flush(lfs3_t *lfs3, lfs3_file_t *file) {
1345813458
if (lfs3_o_isunflush(file->b.h.flags)) {
1345913459
#ifdef LFS3_KVONLY
1346013460
err = lfs3_file_flushset_(lfs3, file,
13461-
file->cache.d.u.buffer_, file->cache.d.size);
13461+
file->cache.buffer, file->cache.size);
1346213462
if (err) {
1346313463
goto failed;
1346413464
}
1346513465
#else
1346613466
err = lfs3_file_flush_(lfs3, file,
13467-
file->cache.pos, file->cache.d.u.buffer_, file->cache.d.size);
13467+
file->cache.pos, file->cache.buffer, file->cache.size);
1346813468
if (err) {
1346913469
goto failed;
1347013470
}
@@ -13504,6 +13504,7 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1350413504
lfs3_data_t name_data;
1350513505
lfs3_rattr_t shrub_rattrs[1];
1350613506
lfs3_size_t shrub_rattr_count = 0;
13507+
lfs3_data_t file_data;
1350713508
lfs3_shrubcommit_t shrub_commit;
1350813509

1350913510
// uncreated files must be unsync
@@ -13547,7 +13548,7 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1354713548
#ifndef LFS3_KVONLY
1354813549
LFS3_ASSERT(file->cache.pos == 0);
1354913550
#endif
13550-
LFS3_ASSERT(file->cache.d.size == lfs3_file_size_(file));
13551+
LFS3_ASSERT(file->cache.size == lfs3_file_size_(file));
1355113552

1355213553
// discard any lingering bshrub state
1355313554
#ifndef LFS3_KVONLY
@@ -13556,10 +13557,13 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1355613557
lfs3_file_discardbshrub(file);
1355713558

1355813559
// build a small shrub commit
13559-
if (file->cache.d.size > 0) {
13560+
if (file->cache.size > 0) {
13561+
file_data = LFS3_DATA_BUF(
13562+
file->cache.buffer,
13563+
file->cache.size);
1356013564
shrub_rattrs[shrub_rattr_count++] = LFS3_RATTR_DATA(
13561-
LFS3_TAG_DATA, +file->cache.d.size,
13562-
&file->cache.d);
13565+
LFS3_TAG_DATA, +file->cache.size,
13566+
&file_data);
1356313567

1356413568
LFS3_ASSERT(shrub_rattr_count
1356513569
<= sizeof(shrub_rattrs)/sizeof(lfs3_rattr_t));
@@ -13695,14 +13699,14 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1369513699
//
1369613700
// note we need to be careful if caches have different
1369713701
// sizes, prefer the most recent data in this case
13698-
lfs3_size_t d = file->cache.d.size - lfs3_min(
13702+
lfs3_size_t d = file->cache.size - lfs3_min(
1369913703
lfs3_file_cachesize(lfs3, file_),
13700-
file->cache.d.size);
13704+
file->cache.size);
1370113705
file_->cache.pos = file->cache.pos + d;
13702-
lfs3_memcpy(file_->cache.d.u.buffer_,
13703-
file->cache.d.u.buffer_ + d,
13704-
file->cache.d.size - d);
13705-
file_->cache.d.size = file->cache.d.size - d;
13706+
lfs3_memcpy(file_->cache.buffer,
13707+
file->cache.buffer + d,
13708+
file->cache.size - d);
13709+
file_->cache.size = file->cache.size - d;
1370613710

1370713711
// update any custom attrs
1370813712
for (lfs3_size_t i = 0; i < file->cfg->attr_count; i++) {
@@ -13778,10 +13782,10 @@ int lfs3_file_sync(lfs3_t *lfs3, lfs3_file_t *file) {
1377813782
// though don't flush quite yet if our file is small and can be
1377913783
// combined with sync in a single commit
1378013784
int err;
13781-
if (!(file->cache.d.size == lfs3_file_size_(file)
13782-
&& file->cache.d.size <= lfs3->cfg->inline_size
13783-
&& file->cache.d.size <= lfs3->cfg->fragment_size
13784-
&& file->cache.d.size < lfs3->cfg->crystal_thresh)) {
13785+
if (!(file->cache.size == lfs3_file_size_(file)
13786+
&& file->cache.size <= lfs3->cfg->inline_size
13787+
&& file->cache.size <= lfs3->cfg->fragment_size
13788+
&& file->cache.size < lfs3->cfg->crystal_thresh)) {
1378513789
err = lfs3_file_flush(lfs3, file);
1378613790
if (err) {
1378713791
goto failed;
@@ -13991,12 +13995,12 @@ int lfs3_file_truncate(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t size_) {
1399113995
}
1399213996

1399313997
// truncate our cache
13994-
file->cache.d.size = lfs3_min(
13995-
file->cache.d.size,
13998+
file->cache.size = lfs3_min(
13999+
file->cache.size,
1399614000
size_ - lfs3_min(file->cache.pos, size_));
1399714001
file->cache.pos = lfs3_min(file->cache.pos, size_);
1399814002
// mark as flushed if this completely truncates our cache
13999-
if (file->cache.d.size == 0) {
14003+
if (file->cache.size == 0) {
1400014004
lfs3_file_discardcache(file);
1400114005
}
1400214006

@@ -14073,27 +14077,27 @@ int lfs3_file_fruncate(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t size_) {
1407314077
}
1407414078

1407514079
// fruncate our cache
14076-
lfs3_memmove(file->cache.d.u.buffer_,
14077-
&file->cache.d.u.buffer_[lfs3_min(
14080+
lfs3_memmove(file->cache.buffer,
14081+
&file->cache.buffer[lfs3_min(
1407814082
lfs3_smax(
1407914083
size - size_ - file->cache.pos,
1408014084
0),
14081-
file->cache.d.size)],
14082-
file->cache.d.size - lfs3_min(
14085+
file->cache.size)],
14086+
file->cache.size - lfs3_min(
1408314087
lfs3_smax(
1408414088
size - size_ - file->cache.pos,
1408514089
0),
14086-
file->cache.d.size));
14087-
file->cache.d.size -= lfs3_min(
14090+
file->cache.size));
14091+
file->cache.size -= lfs3_min(
1408814092
lfs3_smax(
1408914093
size - size_ - file->cache.pos,
1409014094
0),
14091-
file->cache.d.size);
14095+
file->cache.size);
1409214096
file->cache.pos -= lfs3_smin(
1409314097
size - size_,
1409414098
file->cache.pos);
1409514099
// mark as flushed if this completely truncates our cache
14096-
if (file->cache.d.size == 0) {
14100+
if (file->cache.size == 0) {
1409714101
lfs3_file_discardcache(file);
1409814102
}
1409914103

‎lfs3.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,6 @@ typedef struct lfs3_data {
636636
lfs3_size_t size;
637637
union {
638638
const uint8_t *buffer;
639-
uint8_t *buffer_;
640639
struct {
641640
lfs3_block_t block;
642641
lfs3_size_t off;
@@ -742,11 +741,14 @@ typedef struct lfs3_file {
742741
#endif
743742

744743
// in-RAM cache
744+
//
745+
// note this lines up with lfs3_data_t's buffer representation
745746
struct {
746747
#ifndef LFS3_KVONLY
747748
lfs3_off_t pos;
748749
#endif
749-
lfs3_data_t d;
750+
lfs3_off_t size;
751+
uint8_t *buffer;
750752
} cache;
751753

752754
// on-disk leaf bptr

0 commit comments

Comments
 (0)