Skip to content

Commit 940cb18

Browse files
diondokterdiondokter
authored
Merge pull request #82 from tweedegolf/queue-perf
Queue perf
2 parents 58cd49a + 18c21dd commit 940cb18

File tree

9 files changed

+181
-68
lines changed

9 files changed

+181
-68
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44

55
## Unreleased
66

7+
- Fixed queue regression introduced in 2.0.0. It used to erase pages opportunistically for better performance.
8+
This was missed in a refactor. This feature is back now and unlocks a boatload of performance.
9+
710
## 4.0.2 17-06-25
811

912
- Move to edition 2024

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,8 @@ These numbers are taken from the test cases in the cache module:
9797
| Name | RAM bytes | Map # flash reads | Map flash bytes read | Queue # flash reads | Queue flash bytes read |
9898
| ---------------: | -------------------------------------------: | ----------------: | -------------------: | ------------------: | ---------------------: |
9999
| NoCache | 0 | 100% | 100% | 100% | 100% |
100-
| PageStateCache | 1 * num pages | 77% | 97% | 51% | 90% |
101-
| PagePointerCache | 9 * num pages | 70% | 89% | 35% | 61% |
100+
| PageStateCache | 1 * num pages | 77% | 97% | 41% | 85% |
101+
| PagePointerCache | 9 * num pages | 70% | 89% | 6% | 14% |
102102
| KeyPointerCache | 9 * num pages + (sizeof(KEY) + 4) * num keys | 6.2% | 8.2% | - | - |
103103

104104
#### Takeaways

‎fuzz/Cargo.toml‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@ edition = "2021"
88
cargo-fuzz = true
99

1010
[dependencies]
11-
libfuzzer-sys = "0.4"
11+
libfuzzer-sys = "0.4.9"
1212
sequential-storage = { path = "..", features = ["_test"] }
13-
arbitrary = { version = "1.2.2", features = ["derive"] }
14-
rand = "0.8.5"
15-
rand_pcg = "0.3.1"
16-
futures = { version = "0.3.30", features = ["executor"] }
13+
arbitrary = { version = "1.4.1", features = ["derive"] }
14+
rand = "0.9.1"
15+
rand_pcg = "0.9.0"
16+
futures = { version = "0.3.31", features = ["executor"] }
1717

1818
# Prevent this from interfering with workspaces
1919
[workspace]

‎fuzz/fuzz_targets/map.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
114114
}
115115
Err(Error::FullStorage) => {}
116116
Err(Error::Storage {
117-
value: MockFlashError::EarlyShutoff(_),
117+
value: MockFlashError::EarlyShutoff(_, _),
118118
backtrace: _backtrace,
119119
}) => {
120120
match block_on(sequential_storage::map::fetch_item::<u8, &[u8], _>(
@@ -165,7 +165,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
165165
assert_eq!(None, map.get(&key));
166166
}
167167
Err(Error::Storage {
168-
value: MockFlashError::EarlyShutoff(_),
168+
value: MockFlashError::EarlyShutoff(_, _),
169169
backtrace: _backtrace,
170170
}) => {
171171
#[cfg(fuzzing_repro)]
@@ -193,7 +193,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
193193
map.remove(&key);
194194
}
195195
Err(Error::Storage {
196-
value: MockFlashError::EarlyShutoff(_),
196+
value: MockFlashError::EarlyShutoff(_, _),
197197
backtrace: _backtrace,
198198
}) => {
199199
// Check if the item is still there. It might or it might not and either is fine
@@ -238,7 +238,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
238238
map.clear();
239239
}
240240
Err(Error::Storage {
241-
value: MockFlashError::EarlyShutoff(_),
241+
value: MockFlashError::EarlyShutoff(_, _),
242242
backtrace: _backtrace,
243243
}) => {
244244
for key in map.keys().copied().collect::<Vec<_>>() {

‎fuzz/fuzz_targets/queue.rs‎

Lines changed: 54 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use libfuzzer_sys::fuzz_target;
66
use rand::{Rng, SeedableRng};
77
use sequential_storage::{
88
cache::{CacheImpl, NoCache, PagePointerCache, PageStateCache},
9-
mock_flash::{MockFlashBase, MockFlashError, WriteCountCheck},
9+
mock_flash::{MockFlashBase, MockFlashError, Operation, WriteCountCheck},
1010
Error,
1111
};
1212
use std::{collections::VecDeque, fmt::Debug, ops::Range};
@@ -79,7 +79,9 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
7979

8080
match &mut op {
8181
Op::Push(op) => {
82-
let val: Vec<u8> = (0..op.value_len as usize % 16).map(|_| rng.gen()).collect();
82+
let val: Vec<u8> = (0..op.value_len as usize % 16)
83+
.map(|_| rng.random())
84+
.collect();
8385

8486
let max_fit = match block_on(sequential_storage::queue::find_max_fit(
8587
&mut flash,
@@ -124,7 +126,7 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
124126
}
125127
}
126128
Err(Error::Storage {
127-
value: MockFlashError::EarlyShutoff(address),
129+
value: MockFlashError::EarlyShutoff(address, _),
128130
backtrace: _backtrace,
129131
}) => {
130132
// We need to check if it managed to write
@@ -164,17 +166,19 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
164166
);
165167
}
166168
Err(Error::Storage {
167-
value: MockFlashError::EarlyShutoff(address),
169+
value: MockFlashError::EarlyShutoff(address, operation),
168170
backtrace: _backtrace,
169171
}) => {
170172
#[cfg(fuzzing_repro)]
171173
eprintln!(
172174
"Early shutoff when popping (single)! Originated from:\n{_backtrace:#}"
173175
);
174176

175-
if !matches!(block_on(flash.get_item_presence(address)), Some(true)) {
176-
// The item is no longer readable here
177-
order.pop_front();
177+
if operation != Operation::Erase {
178+
if !matches!(block_on(flash.get_item_presence(address)), Some(true)) {
179+
// The item is no longer readable here
180+
order.pop_front();
181+
}
178182
}
179183
}
180184
Err(Error::Corrupted {
@@ -202,6 +206,17 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
202206
order.front().as_ref().map(|target| target.as_slice())
203207
);
204208
}
209+
Err(Error::Storage {
210+
value: MockFlashError::EarlyShutoff(_, Operation::Erase),
211+
backtrace: _backtrace,
212+
}) => {
213+
#[cfg(fuzzing_repro)]
214+
eprintln!(
215+
"Early shutoff when getting next iterator entry! Originated from:\n{_backtrace:#}"
216+
);
217+
218+
break;
219+
}
205220
Err(Error::Corrupted {
206221
backtrace: _backtrace,
207222
}) => {
@@ -243,6 +258,9 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
243258
);
244259

245260
if *do_pop {
261+
#[cfg(fuzzing_repro)]
262+
eprintln!("Popping item at address: {}", value.address());
263+
246264
let popped = block_on(value.pop());
247265

248266
match popped {
@@ -268,20 +286,22 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
268286
panic!("Corrupted!");
269287
}
270288
Err(Error::Storage {
271-
value: MockFlashError::EarlyShutoff(address),
289+
value: MockFlashError::EarlyShutoff(address, operation),
272290
backtrace: _backtrace,
273291
}) => {
274292
#[cfg(fuzzing_repro)]
275293
eprintln!(
276294
"Early shutoff when popping iterator entry! Originated from:\n{_backtrace:#}"
277295
);
278296

279-
if !matches!(
280-
block_on(flash.get_item_presence(address)),
281-
Some(true)
282-
) {
283-
// The item is no longer readable here
284-
order.remove(i - popped_items).unwrap();
297+
if operation != Operation::Erase {
298+
if !matches!(
299+
block_on(flash.get_item_presence(address)),
300+
Some(true)
301+
) {
302+
// The item is no longer readable here
303+
order.remove(i - popped_items).unwrap();
304+
}
285305
}
286306

287307
break;
@@ -293,6 +313,25 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
293313
Ok(None) => {
294314
assert_eq!(None, order.get(i as usize - popped_items));
295315
}
316+
Err(Error::Storage {
317+
value: MockFlashError::EarlyShutoff(address, operation),
318+
backtrace: _backtrace,
319+
}) => {
320+
#[cfg(fuzzing_repro)]
321+
eprintln!(
322+
"Early shutoff when getting next iterator entry! Originated from:\n{_backtrace:#}"
323+
);
324+
325+
if operation != Operation::Erase {
326+
if !matches!(block_on(flash.get_item_presence(address)), Some(true))
327+
{
328+
// The item is no longer readable here
329+
order.remove(i - popped_items).unwrap();
330+
}
331+
}
332+
333+
break;
334+
}
296335
Err(Error::Corrupted {
297336
backtrace: _backtrace,
298337
}) => {
@@ -302,7 +341,7 @@ fn fuzz(ops: Input, mut cache: impl CacheImpl + Debug) {
302341
);
303342
panic!("Corrupted!");
304343
}
305-
Err(e) => panic!("Error iterating queue: {e:?}"),
344+
Err(e) => panic!("Error iterating queue: {e:#?}"),
306345
}
307346
}
308347
}

‎src/cache/tests.rs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ mod queue_tests {
1919
assert_eq!(
2020
run_test(&mut NoCache::new()).await,
2121
FlashStatsResult {
22-
erases: 146,
23-
reads: 594934,
22+
erases: 149,
23+
reads: 165009,
2424
writes: 6299,
25-
bytes_read: 2766058,
25+
bytes_read: 651212,
2626
bytes_written: 53299
2727
}
2828
);
@@ -33,10 +33,10 @@ mod queue_tests {
3333
assert_eq!(
3434
run_test(&mut PageStateCache::<NUM_PAGES>::new()).await,
3535
FlashStatsResult {
36-
erases: 146,
37-
reads: 308740,
36+
erases: 149,
37+
reads: 68037,
3838
writes: 6299,
39-
bytes_read: 2479864,
39+
bytes_read: 554240,
4040
bytes_written: 53299
4141
}
4242
);
@@ -47,10 +47,10 @@ mod queue_tests {
4747
assert_eq!(
4848
run_test(&mut PagePointerCache::<NUM_PAGES>::new()).await,
4949
FlashStatsResult {
50-
erases: 146,
51-
reads: 211172,
50+
erases: 149,
51+
reads: 9959,
5252
writes: 6299,
53-
bytes_read: 1699320,
53+
bytes_read: 89616,
5454
bytes_written: 53299
5555
}
5656
);

‎src/lib.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,9 @@ async fn try_general_repair<S: NorFlash>(
9393
}
9494
}
9595

96+
#[cfg(fuzzing_repro)]
97+
eprintln!("General repair has been called");
98+
9699
Ok(())
97100
}
98101

‎src/mock_flash.rs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -92,16 +92,16 @@ impl<const PAGES: usize, const BYTES_PER_WORD: usize, const PAGE_WORDS: usize>
9292
}
9393
}
9494

95-
fn check_shutoff(&mut self, address: u32, _operation: &str) -> Result<(), MockFlashError> {
95+
fn check_shutoff(&mut self, address: u32, operation: Operation) -> Result<(), MockFlashError> {
9696
if let Some(bytes_until_shutoff) = self.bytes_until_shutoff.as_mut() {
9797
if let Some(next) = bytes_until_shutoff.checked_sub(1) {
9898
*bytes_until_shutoff = next;
9999
Ok(())
100100
} else {
101101
#[cfg(fuzzing_repro)]
102-
eprintln!("!!! Shutoff at {address} while doing '{_operation}' !!!");
102+
eprintln!("!!! Shutoff at {address} while doing '{operation:?}' !!!");
103103
self.bytes_until_shutoff = None;
104-
Err(MockFlashError::EarlyShutoff(address))
104+
Err(MockFlashError::EarlyShutoff(address, operation))
105105
}
106106
} else {
107107
Ok(())
@@ -297,7 +297,7 @@ impl<const PAGES: usize, const BYTES_PER_WORD: usize, const PAGE_WORDS: usize> N
297297
}
298298

299299
for index in from..to {
300-
self.check_shutoff(index as u32, "erase")?;
300+
self.check_shutoff(index as u32, Operation::Erase)?;
301301
self.as_bytes_mut()[index] = u8::MAX;
302302

303303
if index % BYTES_PER_WORD == 0 {
@@ -328,7 +328,7 @@ impl<const PAGES: usize, const BYTES_PER_WORD: usize, const PAGE_WORDS: usize> N
328328
.zip(range.step_by(BYTES_PER_WORD))
329329
{
330330
for (byte_index, byte) in source_word.iter().enumerate() {
331-
self.check_shutoff((address + byte_index) as u32, "write")?;
331+
self.check_shutoff((address + byte_index) as u32, Operation::Write)?;
332332

333333
if byte_index == 0 {
334334
let word_writable = &mut self.writable[address / BYTES_PER_WORD];
@@ -371,7 +371,7 @@ pub enum MockFlashError {
371371
/// Location not writeable.
372372
NotWritable(u32),
373373
/// We got a shutoff
374-
EarlyShutoff(u32),
374+
EarlyShutoff(u32, Operation),
375375
}
376376

377377
impl NorFlashError for MockFlashError {
@@ -380,7 +380,7 @@ impl NorFlashError for MockFlashError {
380380
MockFlashError::OutOfBounds => NorFlashErrorKind::OutOfBounds,
381381
MockFlashError::NotAligned => NorFlashErrorKind::NotAligned,
382382
MockFlashError::NotWritable(_) => NorFlashErrorKind::Other,
383-
MockFlashError::EarlyShutoff(_) => NorFlashErrorKind::Other,
383+
MockFlashError::EarlyShutoff(_, _) => NorFlashErrorKind::Other,
384384
}
385385
}
386386
}
@@ -550,3 +550,11 @@ impl approx::RelativeEq for FlashAverageStatsResult {
550550
.relative_eq(&other.avg_bytes_written, epsilon, max_relative)
551551
}
552552
}
553+
554+
#[allow(missing_docs)]
555+
#[derive(Debug, Clone, PartialEq, Eq)]
556+
pub enum Operation {
557+
Read,
558+
Write,
559+
Erase,
560+
}

0 commit comments

Comments
 (0)