This adds -i/--internal to ctx.py and structs.py, which has proven
useful for introspection/debugging. Being able to view the ctx/args of
internal functions is nice, even if they don't actually contribute to
the high-level cost.
This also reverts structs.py to limit to .h files by default, to match
ctx.py, once again relying on dwarf file info. This has been a bit
unreliable in the past, but there's not much else that determines if a
struct is part of the "public interface" in C.
But that's what ctx.py is for.
---
Also fixed an issue where structs appearing in multiple files would have
their sizes added together, which ends up with some pretty confusing
results (sizeof(uint32_t) => 8?).
This can be explicitly disabled with -x/--no-strip in the relevant
scripts, but stripping by default seems to be more useful for composing
results in higher-level scripts. It's better for the result names to be
consistent, even if they don't match the .o symbols exactly.
Note some scripts are unaffected:
- cov.py - gcov doesn't seem to have an option for getting the
unstripped symbols, so we only output the stripped names.
- structs.py - structs.py deals with struct names, which are notably not
symbols.
Now that I'm looking into some higher-level scripts, being able to merge
results without first renaming everything is useful.
This gives most scripts an implicit prefix for field fields, but _not_
by fields, allowing easy merging of results from different scripts:
$ ./scripts/stack.py lfs.ci -o-
function,stack_frame,stack_limit
lfs_alloc,288,1328
lfs_alloc_discard,8,8
lfs_alloc_findfree,16,32
...
At least now these have better support in scripts with the addition of
the --prefix flag (this was tricky for csv.py), which allows explicit
control over field field prefixes:
$ ./scripts/stack.py lfs.ci -o- --prefix=
function,frame,limit
lfs_alloc,288,1328
lfs_alloc_discard,8,8
lfs_alloc_findfree,16,32
...
$ ./scripts/stack.py lfs.ci -o- --prefix=wonky_
function,wonky_frame,wonky_limit
lfs_alloc,288,1328
lfs_alloc_discard,8,8
lfs_alloc_findfree,16,32
...
So:
$ ./scripts/code.py lfs.o -o- -q
Becomes:
$ ./scripts/code.py lfs.o -o-
The original intention of -o/-O _not_ being exclusive (aka table is
still rendered unless disabled with -q/--quiet), was to allow results to
be written to csv files and rendered to tables in a single pass.
But this was never useful. Heck, we're not even using this in our
Makefile right now because it would make the rule dependencies more
complicated than it's worth. Even for long-running result scripts
(perf.py, perfbd.py, etc), most of the work is building that csv file,
the cost of rendering a table in a second pass is negligible.
In every case I've used -o/-O, I've also wanted -q/--quiet, and almost
always forget this on the first run. So might as well make the expected
behavior the actual behavior.
---
As a plus, this let us simplify some of the scripts a bit, by replacing
visibility filters with -o/-O dependent by-fields.
This makes it so scripts with complex fields will still output all
fields to output csv/json files, while only showing a user-friendly
subset unless -f/--field is explicitly provided.
While internal fields are often too much information to show by default,
csv/json files are expected to go to other scripts, not humans. So more
information is more useful up until you actually hit a performance
bottleneck.
And if you _do_ somehow manage to hit a performance bottleneck, you can
always limit the output with explicit -f/--field flags.
With this, we apply the same result modifiers (exprs/defines/hot/etc) to
both the input results and -d/--diff results. So if both start with the
same format, diffing/hotifying/etc should work as expected.
This is really the only way I can seen -d/--diff results working with
result modifiers in a way that makes sense.
The downside of this is that you can't save results with some complex
operation applied, and then diff while applying the same operation,
since most of the newer operations (hotify) are _not_ idempotent.
Fortunately the two alternatives are not unreasonable:
1. Save results _without_ the operation applied, since the operation
will be applied to both the input and diff results.
This is a bit asymmetric, but should work.
2. Apply the operation to the input and then pipe to csv.py for diffing.
This used to "just work" when we did _not_ apply operations to output
csv/json, but this was really just equivalent to 1..
I think the moral of the story is you can solve any problem with enough
chained csv.py calls.
It's just too unintuitive to filter after exprs.
Note this is consistent with how exprs/mods are evaluated. Exprs/mods
can't reference other exprs/mods because csv.py is only single-pass, so
allowing defines to reference exprs/mods is surprising.
And the solution to needing these sort of post-expr/mod references is
the same for defines: You can always chain multiple csv.py calls.
The reason defines were change to evaluate after expr eval was because
this seemed inconsistent with other result scripts, but this is not
actually the case. Other result scripts simply don't have exprs/mods, so
filtering in fold is the same as filtering during collection. Note that
even in fold, filtering is done _before_ the actual fold/sum operation.
---
Also fixed a recursive-define regression when folding. Counter-
intuitively, we _don't_ want to recursively apply define filters. If we
do the results will just end up too confusing to be useful.
This should either have checked diff_result==None, or we should be
mapping diff_result=None => diff_result_=None. To be safe I've done
both.
This was a nasty typo and I only noticed because ctx.py stopped printing
"cycle detected" for our linked-lists (which are expected to be cyclic).
It felt weird that adding hidden fields required changing existing
flags unrelated to the field you actually want to affect, and the
upper/lower flag thing seems to work well for -s/-S sooo...
- Replaced -l/--label with -B/--hidden-by for by fields that can
be hidden from the table renderer.
- Added -F/--hidden-field as a similar thing for field fields.
- Better integrated -i/--enumerate into by fields, now these actually
maintain related order. And of course added a matching
-I/--hidden-enumerate flag.
The only downside is this is eating a lot of flag names.. But one of the
nice thing about limiting this complexity to csv.py is it avoids these
flag names cluttering up the other result scripts.
---
The -F/--hidden-fields flag I'm not so sure about, since field exprs
can't really reference each other (single pass). But it does provide
symmetry with -B/--hidden-by, and reserves the name in case hidden field
fields are more useful in the future.
Unfortunately it _is_ annoyingly inconsistent with other hidden fields
(-S/--sort, -D/--define, etc) in that it does end up in output csvs...
But this script is already feeling way over-engineered as is.
Kind of a complicated corner case, but this shows up if you try to sort
by fields as numbers and not as strings. In theory this is possible by
creating a hidden sort field with a typed expr:
$ ./scripts/csv.py test.csv -bi -bfunction -Si=i
But we weren't typechecking sort fields that already exist in the by
fields, since these are usually strings.
This fix is to make sure all exprs are in the typechecked fields, even
if they are already in by fields. There's no real cost to this.
---
Note this version does _not_ typecheck i, and sorts by string:
$ ./scripts/csv.py test.csv -bi -bfunction -Si
This raises the question, should we always sort by string by default?
I don't think so. It's easy to miss the difference, and a typecheck
error is a lot safer than incorrect sorting.
So this will sort by number, with i as a hidden field:
$ ./scripts/csv.py test.csv -bfunction -Si
If you want to sort by string with a hidden field, this is still
possible with -l/--label:
$ ./scripts/csv.py test.csv -bi -lfunction -Si
There's an ordering issue with hotifying and folding when we have
multiple foldable results with children. This was hard to notice since
most of the recursive scripts have unique results, but it _is_ an issue
for perf.py/perfbd.py, which rely on result folding to merge samples.
The fix is to fold _before_ hotifying.
We could fold multiple times to avoid changing the behavior of the
result scripts, but instead I've just moved the folding in the table
renderer up into the relevant main functions. This means 1. we only fold
once, and 2. folding affects outputted csv/json files.
I'm a bit on the fence about this behavior change, but it is a bit more
consistent with how -r/--hot, -z/--depth, etc, affect both table and
csv/json results consistently.
Maybe we should move towards the table render always reflecting the
csv/json results? Most csv/json usage is with -q/--quiet anyways...
---
This does create a new risk in that the table renderer can hide results
if they aren't folded first.
To hopefully avoid this I've added an assert in the table renderer if it
notices results being hidden.
I guess in addition to its other utilities, csv.py is now also turning
into a sort of man database for some of the more complicated APIs in the
scripts:
./csv.py --help
./csv.py --help-exprs
./csv.py --help-mods
It's a bit minimal, but better than nothing.
Also dropped the %c modifier because this never actually worked.
This gives csv.py access to a hidden feature in our table renderer used
by some of the other scripts: fields that affect by-field grouping, but
aren't actually printed.
For example, this prevents summing same named functions in different
files, but only shows the function name in the table render:
$ ./scripts/csv.py lfs.code.csv -bfile -bfunction -lfunction
function size
lfs_alloc 398
lfs_alloc_discard 31
lfs_alloc_findfree 77
...
This is especially useful when enumerating results. For example, this
prevents any summing without extra table noise:
$ ./scripts/csv.py lfs.code.csv -i -bfunction -fsize -lfunction
function size
lfs_alloc 398
lfs_alloc_discard 31
lfs_alloc_findfree 77
...
I also tweaked -b/--by field defaults a bit to account to
enumerate/label fields a bit better.
This removes most of the special behavior around how -r/--hot and
-i/--enumerate interact. This does mean -r/--hot risks folding results
if -i/--enumerate is not specified, but this is _technically_ a valid
operation.
For most of the recursive result scripts, I've replaced the "i" field
with separate "z" and "i" fields for depth and field number, which I
think is a bit more informative/useful.
I've also added a default-hidden "off" field to structs.py/ctx.py, since
we have that info available. I considered replacing "i" with this, but
decided against it since non-zero offsets for union members would risk
being confusing/mistake prone.
Guh
This may have been more work than I expected. The goal was to allowing
passing recursive results (callgraph info, structs, etc) between
scripts, which is simply not possible with csv files.
Unfortunately, this raised a number of questions: What happens if a
script receives recursive results? -d/--diff with recursive results?
How to prevent folding of ordered results (structs, hot, etc) in piped
scripts? etc.
And ended up with a significant rewrite of most of the result scripts'
internals.
Key changes:
- Most result scripts now support -O/--output-json in addition to
-o/--json, with -O/--output-json including any recursive results in
the "children" field.
- Most result scripts now support both csv and json as input to relevant
flags: -u/--use, -d/--diff, -p/--percent. This is accomplished by
looking for a '[' as the first character to decide if an input file is
json or csv.
Technically this breaks if your json has leading whitespace, but why
would you ever keep whitespace around in json? The human-editability
of json was already ruined the moment comments were disallowed.
- csv.py requires all fields to be explicitly defined, so added
-i/--enumerate, -Z/--children, and -N/--notes. At least we can provide
some reasonable defaults so you shouldn't usually need to type out the
whole field.
- Notably, the rendering scripts (plot.py, treemapd3.py, etc) and
test/bench scripts do _not_ support json. csv.py can always convert
to/from json when needed.
- The table renderer now supports diffing recursive results, which is
nice for seeing how the hot path changed in stack.py/perf.py/etc.
- Moved the -r/--hot logic up into main, so it also affects the
outputted results. Note it is impossible for -z/--depth to _not_
affect the outputted results.
- We now sort in one pass, which is in theory more efficient.
- Renamed -t/--hot -> -r/--hot and -R/--reverse-hot, matching -s/-S.
- Fixed an issue with -S/--reverse-sort where only the short form was
actually reversed (I misunderstood what argparse passes to Action
classes).
- csv.py now supports json input/output, which is funny.
In addition to providing more functionality for creating -b/--by fields,
this lets us remove strings from the expr parser. Strings had no
well-defined operations and could best be described as an "ugly wart".
Maybe we'll reintroduce string exprs in the future, but for now csv.py's
-f/--field fields will be limited to numeric values.
As an extra plus, no more excessive quoting when injecting new -b/--by
fields.
---
This also fixed sorting on non-field fields, which was apparently
broken. Or at least mostly useless since it was defaulting to string
sorting.
This adopts the Attr rework for the --add-xticklabel and
--add-yticklabel flags.
Sort of.
These require a bit of special behavior to make work, but should at
least be externally consistent with the other Attr flags.
Instead of assigning to by-field groups, --add-xticklabel/yticklabel
assign to the relevant x/y coord:
$ ./scripts/plotmpl.py \
--add-xticklabel='0=zero' \
--add-yticklabel='100=one-hundred'
The real power comes from our % modifiers. As a special case,
--add-xticklabel/yticklabel can reference the special x/y field, which
represents the current x/y coord:
$ ./scripts/plotmpl.py --y2 --yticks=5 --add-yticklabel='%(y)d KiB'
Combined with format specifiers, this allows for quite a bit:
$ ./scripts/plotmpl.py --y2 --yticks=5 --add-yticklabel='0x%(y)04x'
---
Note that plot.py only shows the min/max x/yticks, so plot.py only
accepts indexed --add-xticklabel/yticklabels, and will error if the
assigning variant is used.
Unifying these complicated attr-assigning flags across all the scripts
is the main benefit of the new internal Attr system.
The only tricky bit is we need to somehow keep track of all input fields
in case % modifiers reference fields, when we could previously discard
non-data fields.
Tricky but doable.
Updated flags:
- -L/--label -> -L/--add-label
- --colors -> -C/--add-color
- --formats -> -F/--add-format
- --chars -> -*/--add-char/--chars
- --line-chars -> -_/--add-line-char/--line-chars
I've also tweaked Attr to accept glob matches when figuring out group
assignments. This is useful for matching slightly different, but
similarly named results in our benchmark scripts.
There's probably a clever way to do this by injecting new by fields with
csv.py, but just adding globbing is simpler and makes attr assignment
even more flexible.
No more special indexed attrs at the top-level, now all attrs are
indexed, even if assigned to a specific group.
This just makes it so group-specific cycles are possible:
$ ./scripts/treemap.py -Clfs.c=red -Clfs.c=green
Now, instead of specifying a specific field or comma-separated set of
order-defined constants, -L/--add-label, -C/--add-color, and
-./--add-char/--chars accept a by-field group assignment similar to
-L/--label in plotmpl.py.
I also reworked our % modifiers to behave a bit more like printf
modifiers with optional field targets.
It gets a bit complicated, but this ends up extremely flexible:
- Assign to a specific group:
$ ./scripts/treemap.py -Clfs.c,lfsr_format=orange
- Note this is heirarchical, with more specific groups taking priority:
$ ./scripts/treemap.py -Clfs.c=blue -Clfs.c,lfsr_format=orange
- We can still get the order-assigned behavior by specifying multiple
options, but note there is no longer a comma ambiguity! This is useful
if you want to specify a palette and don't care which dataset gets
which attr:
$ ./scripts/treemap.py -Cred -Cgreen -Cblue
- Mix and match:
$ ./scripts/treemap.py -Cred -Cgreen -Cblue -Clfsr_format=orange
- And with the new % modifiers, we can still use labels stored in a
field:
$ ./scripts/treemap.py -L'%(label_field)s'
- -./--add-char/--chars in treemap.py is a bit of a special case. Since
it only accepts single characters, we can still accept multiple
options with a single flag without having to worry about ambiguities:
$ ./scripts/treemap.py -.asdf
Well, unless you want to include a literal '='. This is possible, but
a bit messy:
$ ./scripts/treemap.py -.as -.=== -.df
Yes that is 3 equal signs... One for argparse, one for the assignment,
one for the '=' literal.
This one is minor, but nice for terseness.
A painful lesson learned from plot[mpl].py: we should never implicitly
sum results in a late-stage rendering script. It just makes it way to
easy to accidentally render incorrect/misleading data, while being
difficult to notice.
We should always render redundant results as redundant results.
If the redundant results are an error, this hopefully makes the problem
more obvious to the user. And if the user really does want summed
results, they can always use csv.py as an intermediate step:
$ ./scripts/treemap.py \
<(./scripts/csv.py lfs.code.csv -bfile -fsize -q -o-)
-fsize
This adds --rectify for a parent-aspect-ratio-preserving --squarify
variant, reverting squarify to try to match the aspect ratio of a
square (1:1).
I can see arguments for both of these. On one hand --squarify makes the
squarest squares, which according to Mark Bruls et al's paper on the
topic is easier visually compare. On the other hand --rectify may be
more visually pleasing and fit into parent tiles better.
d3 allows for any ratio, but at the moment I'm not seeing a strong
reason for the extra parameter.
Like treemap.py, but outputting an svg file, which is quite a bit more
useful.
Things svg is _not_:
- A simple vector graphics format
Things svg _is_:
- A surprisingly powerful high-level graphics language.
I might have to use svgs as an output format more often. It's
surprisingly easy to generate graphics without worrying about low-level
rendering details.
---
Aside from the extra flags for svg details like font, padding,
background colors, etc, the main difference between treemap.py and
treemapd3.py is the addition of the --nested mode, which renders a
containing tile for each recursive group (each -b/--by field).
There's no way --nested would've worked in treemap.py. The main benefit
is the extra labels per subgroup, which are already hard enough to read
in treemap.py.
Other than that, treemapd3.py is mostly the same as treemap.py, but with
a resolution that's actually readable.
Based on the d3 javascript library (https://d3js.org), treemap.py
renders heirarchical data as ascii art:
$ ./scripts/treemap.py lfs.code.csv \
-bfunction -fsize --chars=asdf -W60 -H8
total 65454, avg 369 +-366.8σ, min 3, max 4990
aaaassssddddddaaaadddddssddfffaaadfffaassaassfasssdfdfsddfad
aaaassssddddddaaaadddddssddfffaaadfffaassdfaafasssdfdfsddfsf
aaaassssddddddaaaafffffssddfffsssdaaaddffdfaadfaaasdfafaasfa
aaaassssddddddaaaafffffaaaddddsssaassddffdfaaffssfssfsfadffa
aaaassssffffffssssfffffaaaddddsssaassssffddffffssfdffsadfsad
aaaassssffffffssssaaaaasssffffddfaassssaaassdaaddadffsadadad
aaaassssffffffssssaaaaasssffffddfddffddssassdfassadffsadaffa
aaaassssffffffssssaaaaasssffffddfddffddssassdfaddsdadasfsada
(Normally this is also colored, but you know.)
I've been playing around with d3 to try to better visualize code costs
in littlefs, and it's been quite neat. I figured it would be useful to
directly integrate a similar treemap renderer into our result scripts.
That being said, this ascii rendering is probably too difficult to parse
for any non-trivial data. I'm also working on an svg-based renderer, so
treemap.py is really just for in-terminal previews and an exercise to
understand the underlying algorithms, similar to plot.py/plotmpl.py.
And the related config options:
- cfg->file_buffer_size -> cfg->file_cache_size
- file->cfg->buffer_size -> file->cfg->cache_size
- file->cfg->buffer -> file->cfg->cache_buffer
The original motivation to rename this to file->buffer was to better
align with what other filesystems call this, but I think this is a case
where internal consistency is more important than external consistency.
file->cache better matches lfs->pcache and lfs->rcache, and makes it
easier to read code involving both file->cache and other user-provided
buffers.
Keeping the upstream name also helps with continuity.
You forget one script, running in the background, hogging a whole
core, and suddenly watch's default 2 second sleep time makes a lot more
sense...
One of the main motivators for watch.py _was_ for shorter sleep times,
short enough to render realtime animations (watch is limited to 0.1
seconds for some reason?), but this doesn't mean it needs to be the
default. This can still be accomplished by explicitly specifying
-s/--sleep, and we probably don't want the default to hog all the CPU.
The use case for fast sleeps has been mostly replaced by -k/--keep-open
anyways.
For tailpipe.py and tracebd.py it's a bit less clear, but we probably
don't need to be spamming open calls 10 times a second.
I think passing around rattr.u.datas was undefined behavior, since we
could end up un-unioning it as le32/leb128/lleb128 later.
Taking rattr.u directly in LFSR_RATTR_ means we can't use LFSR_RATTR_ to
create lfsr_rattr_ts from scratch, since rattr.u is not actually a named
type, but we conveniently don't need to do this. And maybe that's a good
thing since it forces the typed variants?
Also renamed rattr.u.data_count -> rattr.u.count now that rattr.u.data
is no longer the default interface in lfsr_rattr_t.
---
Curiously saves a tiny bit of code. If anything I'd have expected a code
increase:
code stack ctx
before: 35536 2440 636
after: 35528 (-0.0%) 2440 (+0.0%) 636 (+0.0%)
This does a couple things:
- Makes attr-lists a bit more self-documenting.
- Adds a bit more type-safety. The LFSR_RATTR_* macros should be able to
reject types that don't match the expected encoding.
- Makes it easier to adjust dsize estimates at one location.
Specifically, this makes it harder to forget bptr's LFSR_BPTR_DSIZE.
---
Surprisingly this did have a small impact on code size. I'm not entirely
sure why, but considering how much of the codebase this touches I'm just
going to chalk this up to compiler noise:
code stack ctx
before: 35488 2440 636
after: 35536 (+0.1%) 2440 (+0.0%) 636 (+0.0%)
lfsr_file_carve seems the hardest hit:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_open 16 20 +4 (+25.0%)
lfsr_file_carve 1316 1356 +40 (+3.0%)
lfsr_remove 408 412 +4 (+1.0%)
TOTAL 35488 35536 +48 (+0.1%)
Mainly just for self-documentation reasons.
This may also make it easier to add LFSR_RATTR_BUF-specific asserts/
tweaks/etc, and helps future refactoring.
But functionally LFSR_RATTR_BUF is equivalent to LFSR_RATTR for now.
No code changes.
Mainly for self-documentation reasons. This is identical to
LFSR_RATTR_LEB128 except for an additional assert in
lfsr_rbyd_appendrattr_.
I was considering removing the little-leb128 concept, but it is still
helping reduce the worst-case size of the various structs we write to
disk. LFSR_BRANCH_DSIZE, for example, contributes heavily to our stack
hot-path.
No code changes.
The only requirement we have for dsize estimates is to help calculate
shrub estimates. So why bother with dsize for attrs that are never
committed to shrubs?
At the moment, the only tags we commit to shrubs are:
- LFSR_TAG_DATA
- LFSR_TAG_BLOCK
- LFSR_TAG_BRANCH
This saves a decent chunk of code:
code stack
before: 35556 2440 636
after: 35488 (-0.2%) 2440 (+0.0%) 636 (+0.0%)
No idea how long this return has been missing, but we should never
ignore a returned err without at least an assert. And this one should
definitely not be an assert.
Code changes minimal:
code stack ctx
before: 35552 2440 636
after: 35556 (+0.0%) 2440 (+0.0%) 636 (+0.0%)
This was the one lazy attr that did _not_ save us code or stack, and in
fact hurt us a bit. Fortunately our lazy attr scheme still allows for
eager attr encoding, so we can revert just this one attr.
The reason is because we need to keep the children branches around as we
recursively commit up the btree, and, even assuming worst-case, the
branch encoding takes up way less RAM than an active rbyd.
Compare for yourself:
in-RAM rbyd: on-disk branch:
.---+---+---+---. .---+- -+- -+- -+- -.
| weight | | block |
+---+---+---+---+ +---+- -+- -+- -+- -'
| blocks | | trunk |
+ + +---+- -+- -+- -+
| | | cksum |
+---+---+---+---+ '---+---+---+---'
|s| trunk |
+---+---+---+---+
|p| eoff |
+---+---+---+---+
| cksum |
'---+---+---+---'
'-------.-------' '---------.---------'
24 bytes 13 bytes
Note we also don't have to care about alignment issues when passing
around the raw encoding.
There may also be something going on with the compiler assuming all
lfsr_rbyd_t pointers may alias, but it's a bit hard to tell.
Saves both code and stack:
code stack ctx
before: 35592 2472 636
after: 35552 (-0.1%) 2440 (-1.3%) 636 (+0.0%)
With the new internal LFSR_RATTR API, there's really no reason to keep
these around.
At one point these were useful for both the implicit lvalues and
automatic buffer size, but GCC's problems with compound-literals and
code size made them almost always backfire.
Now, they're mostly obsolete thanks to the new LFSR_RATTR_* macros.
We do still have a couple LFSR_DATA_* macros (LFSR_DATA_BUF,
LFSR_DATA_SLICE, etc), but these are a bit more fundamental to the
lfsr_data_t type.
This finishes the eager -> lazy attr encoding rework.
Which makes it a good time to look at the total savings from adopting
lazy attr encoding, though there's still a bit of tinkering to do (eager
branches, cksum tags, etc):
code stack ctx
before lazy-attrs: 36280 2576 636
after lazy-attrs: 35592 (-1.9%) 2472 (-4.0%) 636 (+0.0%)
A ~free 688 byte savings in code and 104 bytes in stack is not bad.
Now that rattr.u.cat is no longer in use, the mysterious code size
increase is gone...
Still no idea why this as _any_ impact on code size though:
code stack ctx
before: 35596 2472 636
after: 35592 (-0.0%) 2472 (+0.0%) 636 (+0.0%)
This fully adopts LFSR_RATTR__ and friends:
- LFSR_RATTR -> LFSR_RATTR__ or LFSR_RATTR_DATA__
- LFSR_RATTR_BUF -> LFSR_RATTR__
- LFSR_RATTR_CAT -> LFSR_RATTR_CAT__
- LFSR_RATTR_NOOP -> LFSR_RATTR_NOOP__
- LFSR_RATTR_NAME -> LFSR_RATTR_NAME__
Note the new LFSR_RATTR__ macro also lets us a drop the special rattr
macros, at the cost of a bit less type safety:
- LFSR_RATTR_RATTRS -> LFSR_RATTR__
- LFSR_RATTR_MOVE -> LFSR_RATTR__
- LFSR_RATTR_GRM -> LFSR_RATTR__ (we weren't using this?)
- LFSR_RATTR_SHRUBCOMMIT -> LFSR_RATTR__
Curiously, this ended up adding ~88 bytes to lfsr_file_carve:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_carve 1228 1316 +88 (+7.2%)
lfsr_mdir_commit 2144 2152 +8 (+0.4%)
lfsr_mdir_commit__ 1192 1188 -4 (-0.3%)
lfsr_file_truncate 184 182 -2 (-1.1%)
lfsr_mount 98 96 -2 (-2.0%)
TOTAL 35508 35596 +88 (+0.2%)
I'm really not sure why, all I can think of is maybe the change from a
forced-inline function to a macro added a bunch of compiler noise?
Still, 80 bytes is not worth two competing LFSR_RATTR APIs. Though
it may be worth looking into this in the future.
Total code changes:
code stack ctx
before: 35508 2472 636
after: 35596 (+0.2%) 2472 (+0.0%) 636 (+0.0%)
- LFSR_TAG_BRANCH -+-> lfsr_data_frombranch
- LFSR_TAG_SHRUBBRANCH -'
This was a bit more involved than the others, since it requires changing
what we store in lfsr_bscratch_t.
I did poke around a bit with trying to reduce the total number of rbyd
allocations in lfsr_btree_commit__, but unfortunately we need both the
sibling rbyds and attr rbyds allocated at the same time while we
recursively figure out how to split/merge/commit.
I also almost forgot to include the dsize estimate, but fortunately this
was caught by our tests. We _do_ need branch dsizes, since they
contribute to any non-inlined bshrub's shrub estimate!
---
This was unfortunately a net-negative. We were actually getting a lot of
value from the smaller encoded branch size:
code stack ctx
before: 35472 2440 636
after: 35508 (+0.1%) 2472 (+1.3%) 636 (+0.0%)
It may be worth reverting this, but I want to see how it interacts when
all of the existing eager rattr encoding logic has been removed.
This one is interesting in that we don't just encode to a buffer, but
need to express the concatenation of did + name somehow. Fortunately we
can still leverage the cat circuitry by setting data_count=-2:
- LFSR_TAG_NAME -+-> cat(fromleb128(did), name)
- LFSR_TAG_REG -+
- LFSR_TAG_DIR -+
- LFSR_TAG_STICKYNOTE -'
This does break our shrub estimate for name attrs (currently names have
no technical limit), which would be an issue, but we just happen to never
commit names to shrubs.
In theory you _could_ accurately estimate name attrs if you limited
names to <=(2^15)-5, but I figured this wouldn't be worth the extra code
cost in LFSR_RATTR_NAME__... Especially since it would just go unused...
Saves a nice bit of code, though no stack since we currently don't
allocate any names on the hot-path (lfsr_file_truncate):
code stack ctx
before: 35580 2440 636
after: 35472 (-0.3%) 2440 (+0.0%) 636 (+0.0%)
Note this does _not_ include LFSR_TAG_BOOKMARK, which only contains the
did and can avoid a stack allocation if encoded as a single leb128 attr.
Though breaking up the LFSR_TAG_NAME types does risk a more complicated
switch-case-table...
- LFSR_TAG_GEOMETRY ---> lfsr_data_fromgeometry
Not much to say about this one, LFSR_TAG_GEOMETRY is a bit of an
outlier.
I did consider deduplicating with the mptr encoder, but decided that
would be too hacky, and create problems for future metadata redundancy
things.
Still saves code though, which is nice:
code stack ctx
before: 35632 2440 636
after: 35580 (-0.1%) 2440 (+0.0%) 636 (+0.0%)
- LFSR_TAG_RCOMPAT -+-> lfsr_data_fromle32
- LFSR_TAG_WCOMPAT -+
- LFSR_TAG_OCOMPAT -+
- LFSR_TAG_GCKSUMDELTA -'
- LFSR_TAG_NAMELIMIT -+-> lfsr_data_fromleb128
- LFSR_TAG_FILELIMIT -+
- LFSR_TAG_BOOKMARK -+
- LFSR_TAG_DID -'
This is nice mainly from an internal API standpoint. Single le32/leb128
attrs should be pretty lightweight, and it's nice for the API to reflect
that.
With a bit of tinkering with the internal lfsr_rattr_t type, we can even
pass these directly in the lfsr_rattr_t struct itself, so no need to
keep single le32/leb128 attrs on the stack:
buffer rattr: cat attr: le32/leb128 attr:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| tag |0|size | | tag |1|count| | tag |0|dsize|
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| weight | | weight | | weight |
+---+---+---+---+ .. +---+---+---+---+ .. +---+---+---+---+
| ptr -------. | ptr -------. | le32/leb128 |
'---+---+---+---' | '---+---+---+---' | '---+---+---+---'
.---+---+---+---. | .---+---+---+---. |
| data |<' |mm| size |<'
: : : +---+---+---+---+
| data |
+ +
| |
+---+---+---+---+
|mm| size |
: : :
While tinkering I also ended up renaming a couple things:
- rattr.cat -> rattr.u.datas, rattr.u.buffer, rattr.u.etc
- rattr.count -> rattr.data_count
- added lfsr_rattr_dtag for ignoring on-disk/explicit-data tags
- lfsr_rattr_size -> lfsr_rattr_dsize
Surprisingly very little code savings though. I guess we don't use
single le32/leb128 attrs enough to overcome the added complexity to
lfsr_rbyd_appendrattr_'s switch-case-table?
code stack ctx
before: 35636 2440 636
after: 35632 (-0.0%) 2440 (+0.0%) 636 (+0.0%)
That or there's something else weird going on with this union and
compiler assumptions. Attempting to adopt .u.etc in LFSR_RATTR__ alone
adds ~100 bytes of code, even though both .u.etc and .u.cat are the same
type (const void *)...
Not entirely sure what's going on...
- LFSR_TAG_BLOCK -+-> lfsr_data_frombptr
- LFSR_TAG_SHRUBBLOCK -'
This is where lazy attr encoding causes some problems for our shrub
estimate calculation. As a workaround, lfsr_rattr_t includes the
worst-case encoding size (LFSR_BPTR_DSIZE) in the otherwise-unused count
field, which avoids needing a second tag lookup.
This makes our shrub estimate a little bit worse, but is unavoidable
without reencoding bptr attrs.
On the bright side, this led to a bit of simplication in
lfsr_file_carve.
Saves more code and stack:
code stack ctx
before: 35848 2504 636
after: 35636 (-0.6%) 2440 (-2.6%) 636 (+0.0%)
Christopher Haster