By fields are very different from normal fields in exprs (no type
checking, restricted subexprs, etc), so it makes sense to give them
separate syntaxes to clarify this distinction and improve readability.
This commit adopts optional square brackets for by fields, mimicking
generic/template specialization found in other languages:
Before:
-fx=enumerate()
-fy=enumerate(a,b)
-fz=accumulate(z,a,b)
After:
-fx=enumerate()
-fy=enumerate[a,b]()
-fz=accumulate[a,b](z)
Hopefully the readability argument is pretty obvious.
I went with square brackets to avoid parser ambiguities with <>. To be
honest I've never understood why C++ went with <>, array/function
confusion seems easier to resolve than ambiguous binary/index syntaxes,
but what do I know.
Just by hiding -C/--context, -W/--width, --color from argparse unless
a related flag (-h/--help, -A/--annotate, etc) is found in sys.argv.
This is the same trick we use in test.py/bench.py/perf.py.
---
In other news my litmus test that the scripts work was broken.
This does _not_ error if a script errors:
$ for f in scripts/*.py ; do $f --help ; done
An alternative that works is piping stdout to /dev/null, Python's
exceptions go to stderr by default:
$ for f in scripts/*.py ; do $f --help >/dev/null ; done
This better matches the runners' new -Q/--query-define flag, and, thanks
to some argparse trickery, is simpler implementation wise.
Example:
$ ./scripts/code.py lfs3.o -Qsize
66570
$ ./scripts/stack.py -Qlimit lfs3.ci
3312
The only downside is this takes the --small-table shortform flag, but
--small-table doesn't really need a shortform flag.
The fact that we don't include implicit defines in bench/test output
means we need to query the runner for these surprisingly often. So it'd
be nice to have an easier API than sedding the list output.
Some examples:
$ ./scripts/test.py -QBLOCK_SIZE
4096
32768
$ ./scripts/test.py --query-implicit-define=BLOCK_SIZE
4096
$ ./scripts/test.py --query-permutation-define=BLOCK_SIZE
32768
$ ./scripts/test.py -QBLOCK_SIZZLE
(errors)
Unlike --list-*defines, --query-*defines:
- Separates by newline
- Errors if define is not found
Other than that, --query-*defines uses more-or-less the same code
internally.
Mostly for consistency with other defines. In theory this better maps to
BLOCK_SIZE as a logical multiple of the physical ERASE_SIZE, but the
lack of subblock erasing (what would that even look like?) means this
should have no affect on simulated timings.
It does change the number of erases, however, in case that is useful for
something.
When BENCH_INCLUDE is defined, bench_defines.h should behave like a
normal header file. This includes include guards in case the header file
is included multiple times.
For consistent ordering in later scripts. The previous
-F=min(enumerate()) trick mostly worked, but would get messed up by
running things in parallel (-j).
I've already confused myself a couple times looking at script output,
which is never a good sign.
This reworks bench_rt to write the target file gradually, mixing reads
and writes. This makes it so if the benchmark times out, the results are
still interesting, if less rigorous.
This is useful if you want to compare a change quickly, with more
SIM_TIME leading to a more accurate result.
---
The problem for bench_rt is that we first need to write a file. This can
take _quite_ a while for large SIM_TIME, completely failing in some
cases (bench_rt_many + BENCH_NAND).
The nice thing about bench_wt is that when it fails you still get
interesting numbers out of it. You don't find the relevant throughput
for the given SIZE, but you do find throughput for files _approaching_
SIZE. Unfortunately this didn't carry over to bench_rt.
The good news is the new bench probe system makes it easy to selectively
ignore parts of each bench, so we can rework bench_rt to start with a
CHUNK sized file and gradually increase it until it hits our target
size. This allows bench_rt to also fail gracefully.
Consider a 1 minute run:
$ make bench-runner -j \
&& BENCHFLAGS='bench_rt -DSIM_TIME=60000000000' make bench -j \
&& make bench-marks
bench+probe n t throughput
bench_rt_seq+read 302848 0.0 30690155.6
bench_rt_random+read 302592 0.0 59646605.2
bench_rt_logging+read 52992 5.9 8915.0
bench_rt_many+read 112896 0.2 482831.6
TOTAL 771328 6.2 22707126.8
Vs the default 1 hour run:
$ make bench-runner -j \
&& BENCHFLAGS='bench_rt' make bench -j \
&& make bench-marks
bench+probe n t throughput
bench_rt_seq+read 79600038272 3325.1 23939212.7
bench_rt_random+read 21482635264 3325.0 6461004.1
bench_rt_logging+read 3085056 740.9 4163.9
bench_rt_many+read 706571904 1800.7 392380.4
TOTAL 101792330496 9191.7 7699190.3
The main risk of doing this is cross-contaminating read results with
write operations. Fortunately the current benches appear to be isolated
well enough:
$ make bench-ops
bench+probe readed progged erased
bench_rt_logging+read 86886137 18936138 19767296
bench_rt_seq+read 83127251476 0 0
bench_rt_many+read 45018292401 0 0
bench_rt_random+read 83124213669 0 0
TOTAL 211356643683 18936138 19767296
---
Oh! This also lets us add bench_rt_logging, which needs a mixed writer
to make any sense.
Note bench_rt_logging also includes popping from the log (fifo?), so is
not a strictly read-only bench.
This little per-process counters weren't updated in the move to
cumulative-by-default probes, and were summing already cumulative
results.
I was looking at something like 3 trillion bytes read and was thinking
there was no way that could be right.
The idea here is to try to use the string pointer itself to bypass
strcmps and the O(n) scan.
It doesn't seem to have any impact on our current bench runtime, but it
doesn't hurt to keep around.
This reworks -P/--powerloss to be more consistent with other flexible
flags (-D/--define, -S/--probe, etc):
- Tweaks -P/--powerloss to accept multiple flags (-Pnone -Plinear)
instead of a comma-separated list (-Pnone,linear)
- Adopts an expr-like grammar similar to -Dx='range(3)', -Sx=123shz, etc
(see below)
- Generalizes run_powerloss_linear and run_powerloss_log to accept
start/stop/step conditions, allowing for range and logrange exprs
with minimal work
---
The new expr-like grammar follows what's worked well for -D/--define,
-S/--probe, etc, in which parens can be used to parameterize some of the
more complex scenarios. This makes the -P/--powerloss grammar more
consistent, less ad-hoc, easier to parse, while also providing
flexibility for future powerloss exprs.
As an example, bounded range/logrange variants of linear/log were easy
to add without each needing their own little syntax:
- none -> none - Run with no powerlosses
- linear -> linear - Run with linearly-decreasing powerlosses
- log -> log - Run with exponentially-decreasing pls
- n -> permute(n) - Run all permutations of n powerlosses
- exhaustive -> exhaustive - Run all powerloss permutations
- {1,2,3} -> list(1,2,3) - Run explicit list of powerlosses
- added range(a,b,s) - Run explicit range of powerlosses
- added logrange(a,b,s) - Run explicit range of 2^n powerlosses
- :1248g1 -> :1248g1 - Run custom leb128-encoded set of pls
Note we still keep :-prefixed leb128-encoded powerlosses as is. This is
enough of its own syntax that trying to map it to an expr doesn't really
make sense. And is humorously compatible with most future grammars.
- -S/--probe - Specify a probe to sample.
- -x/--probe-step - Sample probes every n steps.
- --probe-runfreq - Sample probes at this frequency in hz.
- -X/--probe-simfreq - Sample probes at this frequency in simulated hz.
Also:
- --trace-simfreq - Sample trace output at this frequency in
simulated hz.
These give finer grain control over which probes we measure during
benching, and how we measure them.
These also introduce several exciting bench features:
- -S/--probe provides the ability to easily filter which probes you're
interested in at runtime.
This should replace the growing use of MASK defines in the benches.
- -x/--probe-step makes it easy to relax sampling rate when the amount
of data overwhelms later scripts.
This should replace the growing use of STEP defines in the benches.
- The additional concept of simfreq, which allows perf-esque sampling in
simtime. This provides another option for intuitively relaxing probe
sampling rate without sacrificing reproducibility.
(runfreq depends on wall time, so good bye reproducibility, though may
still be useful in interactive contexts.)
Note -S/--probe and -x/--probe-step replace MASK/STEP defines, which
have already proved their usefulness, but required reimplementation in
every bench case. An obvious contender to move into the bench_runner!
---
Note note that -S/--probe also supports some simple sample expressions,
allowing flexible step/simfreq/runfreq at the per-probe level:
- -Swrite=100 - Sample probe "write" every 100 steps
- -Swrite=100rhz - Sample probe "write" 100 times a runtime second
- -Swrite=100shz - Sample probe "write" 100 times a simulated second
Though I wonder how long it will take before I forget this feature
exists.
Adds a set of flags to query the bench_runner for available probes:
- --list-probes - List estimated probes
- --list-suite-probes - List estimated probes for each bench suite
- --list-case-probes - List estimated probes for each bench case
What's fun though, is we don't actually know the bench probes at compile
time, since the BENCH_* macros take a C string. But we're already
preprocessing bench_*.toml with Python, so guessing what probes are
available is easy with a bit of regex:
BENCH_(?:STOP|F?RESULT)\( *"((?:\\.|[^"])*)"
This does make the --list*probes flags best effort, but I think unlikely
to break in practice.
Mainly to make space for some planned bench flags, while also preferring
"step" over "period" (for consistency), and "runfreq" over "freq" (to
differentiate from "simfreq" in the future).
In runners:
- -s/--step -> --step
- --trace-period -> --trace-step
- --trace-freq -> --trace-runfreq
In scripts:
- --record -> -e/--record
- --perf-period -> --perf-step
- --perf-freq -> --perf-runfreq
- --include -> -i/--include
---
One thing that makes this work is the new sys.argv regex trick, where we
try to predict what mode the script will run in by prematching known
mode-switch flags before handing things off to argparse.
Note:
- Hiding flags from argparse risks confusing help-text, so we include
all flags if we see -h/--help in sys.argv.
This doesn't work for the help-text printed if argparse errors, but we
can only do so much. Maybe argparse only showing relevant flags for
the given mode is ok?
- We use -[^-]*[hf].* for shortform flags, which should also match
multiple shortform flags in a single arg (-fhfhfh).
- This requires the conflict_handler='ignore' hack to work, but these
scripts already needed it anyways.
- BENCH_SIMTIME() => lfs3_kiwibd_simtime()
- BENCH_SIMRESET() => lfs3_kiwibd_simreset()
- BENCH_SIMPAUSE() => lfs3_kiwibd_simpause()
- BENCH_SIMRESUME() => lfs3_kiwibd_simresume()
- BENCH_RESET() => lfs3_kiwibd_simreset() + BENCH_STACK/HEAP_RESET()
- BENCH_PAUSE() => lfs3_kiwibd_simpause() + BENCH_STACK/HEAP_PAUSE()
- BENCH_RESUME() => lfs3_kiwibd_simresume() + BENCH_STACK/HEAP_RESUME()
This does two things:
1. Adds pause/resume counters to bd counters to make it easier to
exclude operations from the current bench (potentially useful for
seq+disk usage).
2. Exposes bd simtime operations as BENCH_* macros, to make it a bit
easier to interact with simtime without tying all the benches to
kiwibd. (Not that we'll ever probably not use kiwibd, but still).
Also adopted 32-bit counters for stack/heap pause state instead of a
32-bit stack. Not that either are at a risk of overflowing, but better
safe than sorry.
The point of having separate test/bench runners is to minimize
complexity when different concerns overlap, and the stack/heap
measurements haven't proven necessary for testing yet.
Keeping them around just adds a maintenance burden, and risks messy
interactions with test features if you ever try to turn them on
(heap + powerloss = memory leaks yay).
So removing for now.
If they are useful in the future (cheaper Valgrind-esque checks?),
copying from bench_runner.c -> test_runner.c is super easy.
---
Note these are still available and enabled by default in the bench
runner.
This is a bit awkward due to Makefiles having no concept of boolean ors,
but we can avoid passing multiple -Wl,--wrap flags with enough
ifdefs/ifndefs.
The litmus benches are really only intended for introspection/debugging/
cool plots/etc. They're interesting to poke around with and cover a wide
range of littlefs's data-structures, but are not very rigorous.
To make this more clear for new users, added a new litmus flag for
benches:
litmus = true
This doesn't change anything about how the bench is run, but serves as a
marker to hint that the bench is intended for non-rigorous benchmarking.
---
In the makefile, litmus tests are disabled by default at runtime
(--no-litmus). This is to limit `make bench` to benches that are useful
for performance comparisons.
With --no-litmus at runtime, the litmus benches are at least compiled
into the bench_runner, which should hopefully encourage keeping them up
to date with code changes. Eventually we should also run them in CI, but
only to check for runtime errors.
Unlike our tests, we're not really worried about compile time at the
moment due to how few/small our benches are.
Not sure when this was introduced, but it looks like we were
unintentionally double spacing columns in our table renderer.
The problem is we add spaces for both fields and notes:
a b c d
the_thing 100 (+10%) 200 (+20%) 300 (+30%)
But unconditionally, so if there are no notes (the common case), the
fields end up double-spaced:
a b c d
the_thing 100 200 300
Fixed by checking x[1], and only adding the second space if we have any
notes:
a b c d
the_thing 100 200 300
---
The funny thing is, after using this table renderer for so long, I
assumed the double spacing was intentional.
And maybe it should be? Double-spacing does help visually separate
neighboring columns at the cost of horizontal density. The only problem
being that we really _don't_ have much horizontal density to play with.
Many of our table scripts already run past the 80-col mark just due to
how much data we want to show.
If we do want to double space in the future, we should at least double
space after notes as well for consistency. The current impl appears to
not be able to make up its mind!
This adds -U/--undefine as an inverse -D/--define, allowing you to
select results where a given field does _not_ match a set of
values/globs.
For example, make bench-marks, which need to ignore stack/heap/usage
probes as a special case, can easily filter like so:
$ ./scripts/csv.py test.csv -Uprobe=stack,heap,usage
---
One thing globbing is pretty bad at is inverse matches. This is
_usually_ easy enough to work around, but has been an annoyance enough
times that I think _some_ option to inverse filter is warranted.
I'm not sure -U/--undefine is the best name for this, since field isn't
really "undefined" as a result (well kinda? if you're relying on
implicit by/field rules?), but it gets the job done.
How long has there been a whole key dedicated to percentages sitting on
my keyboard!?
There's some funky business with format strings in argparse, but this
was already worked around for dbgbmap.py's -%/--usage flag.
These are useful for customizing the table renderer's header and total
labels:
./scripts/csv.py test.csv -ba -fc \
-Hc='c(MiB/s)' \
--tlabel='wow so many (%(c)s)'
a c(MiB/s)
x 6
wow so many (6) 6
In theory header labels could be controlled by the field names
themselves, but our use of Python's namedtuples internal is quite
limiting.
The immediate use case is -Hprobe=bench+probe in the bench-related
rules, but it may also be useful for adding units such as in the above
example.
---
I considered adding these to all csv scripts, but decided that was too
much. punescape modifiers are probably a good line for what should be
limited to csv.py.
This flag has proven useful in external scripts, might as well give it a
short form.
-t is also an infrequently used flag, so I think the risk of collision
is low even across all csv scripts. The only existing use is in
test/bench.py for -t/--trace (and apparently in gcov for -t/--stdout?).
The lowercase -c/--compare felt clunky. I think because we tend towards
using uppercase for flags that operate on csv rows, such as -D/--define,
-L/--add-label (plot.py), etc.
This is useful for enforcing an upper-bound on fracs based on their
total component.
Technically possible via decomposing + min + recomposing, but... Well
which one do you think is easier?
- saturate(x)
- frac(min(max(int(x), 0), total(x)), total(x))
And this assumes x is easily available and not some other expr (though
chaining csv.py could work around that).
---
The motivation for this was `make bench-widths`, where one read
benchmark could ruin the entire column due to introducing infinities.
Now:
make bench # (squished a bit)
probe readed progged erased
b_wt_seq+w 1.0/1.0 (100.0%) 31.7/256.0 (12.4%) 4096.0/4096.0 (100.0%)
b_wt_random+w 1.0/1.0 (100.0%) 15.3/256.0 (6.0%) 4096.0/4096.0 (100.0%)
b_wt_logging+w 1.0/1.0 (100.0%) 15.4/256.0 (6.0%) 4096.0/4096.0 (100.0%)
b_wt_many+w 1.0/1.0 (100.0%) 16.1/256.0 (6.3%) 4096.0/4096.0 (100.0%)
b_rt_seq+r 1.0/1.0 (100.0%) 256.0/256.0 (100.0%) 4096.0/4096.0 (100.0%)
b_rt_random+r 1.0/1.0 (100.0%) 256.0/256.0 (100.0%) 4096.0/4096.0 (100.0%)
b_rt_many+r 1.0/1.0 (100.0%) 256.0/256.0 (100.0%) 4096.0/4096.0 (100.0%)
TOTAL 1.0/1.0 (100.0%) 149.0/256.0 (58.2%) 4096.0/4096.0 (100.0%)
# ^- notably not infinity
Based on some experience out-of-tree:
- bench_rbyd - Simple rbyd attr/id litmus benchmark
- bench_btree (new) - Simple btree id/name litmus benchmark
- bench_file (new) - Simple file read/write litmus benchmark
- bench_dir (new) - Simple dir read/write/stat litmus benchmark
- bench_wt - Heavy-duty write-throughput benchmark
- bench_rt (new) - Heavy-duty read-throughput benchmark
Benches take a long time to run for useful results, so we probably don't
want to go crazy with them like with the tests.
Honestly, we may want to chop this down to just the
write/read-throughput benches.
Logging is one of those things that's very useful to keep around, but
has a high-risk of stack/heap costs that shouldn't count towards any
benchmarks (you can always disable logging).
So, lets exclude them from stack/heap measurements.
This could've been done by defining all of littlefs's LFS3_DEBUG/INFO/
WARN/ERROR macros, but intercepting printf directly is a bit less
tedious. As a plus, we eliminate logging costs from any other filesystem
we benchmark, without need to fiddle with everyone's logging APIs.
---
Hmm. Actually, now that I've done a test run, these changes seem to have
no effect.
Which makes sense in hindsight:
1. For efficiencies sake, printf likely tries to allocate infrequently.
Maybe only during the first call?
And we print the bench id before entering the bench.
2. The way our stack measurements work, we only count them if we enter a
bd op or call BENCH_STACK_PAUSE().
So any printfs encountered previously would have been ignored by our
stack measurements.
Still, better safe than sorry.
- Renamed BENCH_STACK/HEAP -> BENCH_STACK/HEAP_WATERMARK
- Renamed BENCH_YES_STACK/HEAP -> BENCH_STACK/HEAP
- Tweaked stack/heap watermarks to hopefully be easier to access when
debugging. Now also exposed as global variables
(bench_stack/heap_watermark).
I considered changing BENCH_STACK/HEAP_WATERMARK to be the variable
itself, to be consistent with TEST_PLS, but decided against it:
1. BENCH_STACK_CURRENT() is a bit magic in that it relies on
__attribute__((noinline)) to force a new stack frame. This wouldn't
really be possible with a variable.
2. TEST_PLS is at least constant from the _current run_'s perspective.
This isn't true for the stack/heap watermarks.
- Reworked internals a bit to hopefully be simpler
Note bench_wt_seq's disk usage is garbage because of the repeated
truncates!
But I figured this is at least useful for the other benches, and we
already have bench_helper_usage. Maybe in the future we'll figure out
some way to get useful disk usage from bench_wt_seq.
Might as well, we have the hooks already.
The only annoying this is these extra probes clutter up the `make
bench-marks` output, so split into two separate rules:
- make bench-marks
- make bench-usage
It's tempting to add disk usage as well (we have bench_helper_usage for
this purpose), but I'm not sure how to measure usage in bench_wt_seq
since it's constantly truncating.
If only for consistency with DISK_GEOMETRY.
The main reason to keep BENCH_PERBYTE around is to help debug/sanity
check the more complex bus+buffer sim. For that purpose it makes sense
to be able to easily switch modes.
The only downside is if it's more difficult to introduce -DDISK_SIM=1 at
runtime vs compile-time, but eh. Consistency wins.
A simple float variant of the CsvFrac type:
- frac(1.5,2) => 1/2 (50.0%)
- ffrac(1.5,2) => 1.5/2.0 (75.0%)
Useful for `make bench-widths` (previously make bench-bus), where we
want to find the average buffer utilization:
probe readed progged erased
b_rbyd+create 1.0/1.0 (100.0%) 13.8/256.0 (5.4%) ∞/4096.0 (∞%)
b_rbyd+delete ∞/1.0 (∞%) ∞/256.0 (∞%) ∞/4096.0 (∞%)
b_rbyd+fetch 1.0/1.0 (100.0%) ∞/256.0 (∞%) ∞/4096.0 (∞%)
b_rbyd+lookup 1.0/1.0 (100.0%) ∞/256.0 (∞%) ∞/4096.0 (∞%)
b_rbyd+usage ∞/1.0 (∞%) ∞/256.0 (∞%) ∞/4096.0 (∞%)
b_wt_seq+w 1.0/1.0 (100.0%) 31.7/256.0 (12.4%) 4096.0/4096.0 (100.0%)
b_wt_random+w 1.0/1.0 (100.0%) 15.3/256.0 (6.0%) 4096.0/4096.0 (100.0%)
b_wt_logging+w 1.0/1.0 (100.0%) 15.4/256.0 (6.0%) 4096.0/4096.0 (100.0%)
b_wt_many+w 1.0/1.0 (100.0%) 16.1/256.0 (6.3%) 4096.0/4096.0 (100.0%)
TOTAL ∞/1.0 (∞%) ∞/256.0 (∞%) ∞/4096.0 (∞%)
Now that we have 4 types, the cast matrix gets a bit complicated, but
this is side-stepped a bit by a custom __frac__ hook.
---
Some other tweaks to csv.py:
- Added CsvFold.type to typecheck folds _after_ we know the expr's final
type.
- Adopted CsvFfrac as an output for most of the math functions/folds
- Stopped early termination of typechecking if we change type!
This was broken: int(float(1.5) + int(1))
This adds __hash__ to CsvFrac, and tweakes __eq__ to be more strict
about equality.
Previously CsvFrac only considered the relevant ratio for equality,
making hashing difficult:
- before: 1/2 == 2/4 => true
- after: 1/2 == 2/4 => false
But now that we have csv.py, with the explicit ratio function, it's
probably a good idea to be strict by default.
Note comparison is unchanged:
- 1/2 < 2/4 => false
- 1/2 > 2/3 => false
---
This popped up during debugging, and would be useful to have around.
Note CsvInt/CsvFloat already implicitly define __hash__ through
namedtuple's implicit __eq__ and friends. But this is disabled in
CsvFrac due to the explicit __eq__.
Which is good because otherwise it would've been wrong with the ratio
comparison!
--no-internal has already proven useful for skipping internal tests for
refactoring, so it makes sense to add --no-reentrant/fuzz flags as well.
--no-fuzz seems particularly useful for when you want to skip the less
targeted fuzz tests:
- with fuzz tests: 634616/634616 passed, in 1239.90s
- with --no-fuzz: 85434/85434 passed, in 423.41s
I also added runtime variants to test/bench_runner and test/bench.py.
These may be useful to skip tests without needing to recompile the
runner.
---
Also tweaked -s/--step to filter permutations in any --list-* flags, for
consistency.
This adds an explicit:
internal = true
As an alternative to:
in = 'lfs3.c'
For marking tests/benches as internal without actually placing them in a
specific source file.
The internal flag and --no-internal have proven suprisingly useful for
running a subset of tests when refactoring, as internal tests break much
more frequently than the high-level API. However, placing all the
internal tests in lfs3.c _has_ put a big strain on compilation/link
times.
`internal = true` now lets you mark tests/benches as internal, without
the extra compile/link overhead. You don't get access to any internal
things, but the flag can still be useful for filtering.
---
The original motivation for this was in the test_fwrite_clip_* tests,
but they ended up using lfs3_bptr_size to check leaf sizes, so oh well.
At least it's a good flag to have around. (In theory these could be made
"fake internal" with a manual bitmask, but it doesn't seem worth the
potential maintenance headache for saving a bit of link time. Though
maybe in the future priorities will change.)
Also cleaned up the handling of None in test/bench config a bit. Now
None should be equivalent to missing config fields, at the cost of more
noise in the Python code. None vs missing always feels unusually clunky
in Python.
Turns out we were using a slightly wrong condition for when to discard
file leaves in lfs3_file_flush_. An unsurprising mistake given size vs
weight subtleties. As a result, it was possible for a write to bypass
the leaf, leaving it with an outdated weight, resulting in an unexpected
hole in the file.
This was surprisingly hard to find as most writes don't leave the leaf
with hole information, only reads.
Fortunately a solution is easy. Just don't use the bptr size here,
instead use the full leaf weight to decide when to discard tracked file
leaves.
Code changes humorously canceling out the Valgrind fix:
code stack ctx
before: 35260 2136 660
after: 35256 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
---
This was found by test_fsync_rwtfrwtf_sparse_fuzz, but only by luck
after the CRYSTAL_THRESH/8 -> CRYSTAL_THRESH/16 tweak.
To prevent a regression, and hopefully catch other bugs like this
(something something cache coherency hard problem), I added a couple
"clip" tests that try to force the cache/leaf bypassing behavior:
- test_fwrite_clip_cache - try clipping the file cache
- test_fwrite_clip_leaf - try clipping the file leaf
- test_fwrite_clip_hole - try clipping the file leaf+hole
test_fwrite_clip_hole does reproduce the bug.
Valgrind was reporting a conditional move on uninitialized read here,
which is correct. If we fetch a data fragment during a read, the
cksize/cksum is meaningless and may be uninitialized.
This was somewhat intentional as both lfs3_bptr_claim and LFS3_o_UNCRYST
are inconsequential when file->leaf is a data fragment. Why bother
checking for a condition that doesn't matter?
But keeping Valgrind happy is significantly more important for
everyone's mental health.
Costs an extra 4 bytes of code:
code stack ctx
before: 35256 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
This has been adopted in external benchmarks for a while, as it manages
to push sequential write performance into a much better region of the
diminishing-returns curve.
But hey! Don't take my word for it, let's see the results from our new
bench_runner for the first time:
NOR throughput cs=1/8 cs=1/16
bench_wt_seq+write 15180.0 29402.6 (+93.7%)
bench_wt_random+write 876.2 957.3 (+9.3%)
bench_wt_logging+write 2001.0 2153.4 (+7.6%)
bench_wt_many+write 453.6 453.6 (+0.0%)
NAND throughput cs=1/8 cs=1/16
bench_wt_seq+write 21778.7 22330.1 (+2.5%)
bench_wt_random+write 3583.0 3637.2 (+1.5%)
bench_wt_logging+write 10855.1 10977.1 (+1.1%)
bench_wt_many+write 68.2 68.2 (+0.0%)
Though this doesn't really capture the tradeoffs related to file tails,
storage usage, etc.
In theory sequential writes are happy to start crystallizing as soon as
any data is written, but this leads to significant waste anytime you're
not going to write most of a block.
This gives us much more room for activities.
It makes sense to keep the test disk small: easier parallelization,
heavier emubd with more test features, and if you're running into space
issues in a test, that usually just means you need to be more creative
with how the test is setup.
But for benches, we're interested what happens when we throw a ton of
data at the system.
Also defaulted to noop erases. 0xff erases behave more predictably,
which is useful for testing. But for benching, less work is faster.
- make bench-ops - Show the amount readed/progged/erased
- make bench-bus - Show average readed/progged/erased per width
These just seem immediately useful for a high-level understanding of
benchmark costs. Especially while trying to understand how things
interact with the new bus+buffer sim.
Turns out mt.isinf is happy to accept non-primitive floats (such as
CsvFloat) as long as __float__ is defined. But CsvInt expects a float
inf, not a CsvFloat, so things explode later.
Fixed by explicitly casting to float if mt.isinf, instead of passing
as-is.
Also tweaked CsvInt/CsvFloat constructors to not bother checking
isinstance, unconditional int/float casts are probably cheaper than the
condition in Python.
To hopefully make it more clear these are test/bench-dependent, and to
try to be consistent with other make bench-runner, bench-list, etc,
rules.
It felt weird to type this without a hyphen now.
Also renamed test/benchmarks-bottlenecks -> test/bench-bottlenecks,
which is considerably less of a mouthful.
Just makes it a bit easier to fiddle with.
I don't think we can adopt the same prefix => define trick for LFS3_*
defines. If we did it'd pull in things like BENCH_CFLAGS, recursively...
This shows an interesting strategy difference between the test_runner
and bench_runner.
In the test_runner we default to the least-stress configuration, to
minimize bugs unrelated to the current test. But the resulting
configuration is unrealistic, as most use cases on flash will probably
want wear-leveling.
In the bench_runner, we should use a more realistic configuration, so
setting BLOCK_RECYCLES=100 by default makes sense.
So now you can easily run multiple/specific geometries without
recompiling the bench runner:
./scripts/bench.py -DDISK_GEOMETRY=0,1
But note by default we only simulate NOR flash. Spitting out multiple
results by default is confusing.
---
Previously this was possible by either compiling multiple bench runners
(with -DBENCH_NAND), or by explicit specifying full the geometry
(-DREAD_SIZE, -DPROG_SIZE, ..., -DREAD_TIMING, ...) at runtime, but both
were clunky and annoying to parameterize.
DISK_GEOMETRY make it easy, fits well with DISK_SIZE, and adds a field
to help identify the geometry in later scripts.
I considered filling out test_defines.h with multiple geometries as
well, but decided against it. The current idea behind test_runner is to
not test specific geometries, but to instead let individual suites/cases
iterate through the specific READ_SIZEs, PROG_SIZEs, etc, that are
relevant. Still, added DISK_GEOMETRY to test_defines.h for consistency,
but it doesn't actually control anything.
After letting it sit for a bit, the previous byte+op sim comes across as
overly clever in a way that is counter-productive. This is highlighted
by erase-timing scaling in a confusing way when per-op.
Fortunately, with a bit of tweaking, we can instead model the bd sim as
separate bus+buffer timings. This seems more intuitive and is closer to
how the actual hardware works.
---
In the bus+buffer model, bd operations are simulated using two sets of
timing estimates:
buffer timings (nor) bus timings (nor)
read_timing (0) readed_timing (40 ns/B)
prog_timing (1563 ns/B) progged_timing (19 ns/B)
erase_timing (10986 ns/B) erased_timing (0)
Bus timings are a simple multiplier of the bytes read/progged/erased,
while buffer timings are rounded up + aligned to the nearest "width":
bd geometry (nor) bd buffers (nor)
read_size (1 B) read_width (1 B)
prog_size (1 B) prog_width (256 B)
erase_size (4096 B) erase_width (4096 B)
For most purposes, the width should just be the device's read/prog/erase
buffer, but I went with the name width to try to keep it generic and
avoid confusion with "buffer" elsewhere in the codebase.
Some notes:
- Like the byte+op sim, the bus+buffer sim allows penalizing small
operations without artificially limiting what operations are possible.
- Because buffer timings depend on read/prog/erase alignment, there's no
simple equation from ops+bytes to bus+buffer. But as a tradeoff, this
new sim more accurately penalizes unaligned operations.
- All timings are still kept as per-byte instead of per-width. This has
proven to be more flexible when benchmarking, as you usually what
timings to scale with the relevant operation.
- Currently this implemented by changing reads/progs/erases to track the
number of "widths" read/progged/erased after alignment. Which makes
the simtime formula roughly:
simtime = reads*read_width*read_timing + readed*readed_timing
(per-butter) (per-bus)
I considered keeping separate counters for calls (read_calls/
prog_calls/erase_calls?), but not sure there's a good reason to. The
theory behind these widths is there no functional difference between
one big call vs multiple width sized calls, though maybe they would be
useful for debugging?
We can always add these later if they turn out to be useful.
- When widths are disable (0), reads/progs/erases reverts to the number
of read/prog/erase calls.
This is the behavior when BENCH_SIMPLE is defined at compile-time.
Now that csv.py's accumulate/delta functions make it easy to switch
between delta/cumulative results, we might as well make the default
results consistent.
The previous difference between n/bench_runtime vs bench_readed/
bench_simtime risked a lot of confusion.
Note we can't use delta results for n, as it doubles as a unique index
for each probe measurement. If we want consistency the only option is
cumulative results. At least that makes the decision easy.
We're explicitly specifying bash in enough rules that adopting it at the
top-level is probably best for minimizing surprises.
Really the only bash feature we need is process substitution
(`a <(b) <(c)`), but it's a hard feature to pass on.
Though it will be interesting to see if any users run into problems with
this.
Christopher Haster