GitLab

Fix marshalling error

Fix an error with marshalling that causes bytes nodes to get written as links if they are written
after a link, because the tag was never reset

traversal.SkipMe feature

Walk functions weren't sufficiently fleshed out.

A few notes about expansion effects due to paths are now made.

General consistency pass across all the methods.

Drop code coverage bot config.

I have little idea what this did,
and found its visualizations more confusing than useful.

I also have little fondness for cloud-based services of this kind
if they don't have any useful contribution to localhost development,
and I think we can easily look at this file and see that it said
nothing of use.

Per the advice of https://github.com/ipld/go-ipld-prime/pull/49#issuecomment-595552836 ...

RIP IT OUT

Promote NodeAssembler/NodeStyle interface rework to core, and use improved basicnode implementation.

This was only useful as a design research expedition.

It's here in history if you want to refer to it.

I'm not sure if there's a name for this way of grouping methods,
but the end result is dotted.Access.Patterns, and it's kinda nice.

This was extensively discussed in the PR:

https://github.com/ipld/go-ipld-prime/pull/49#issuecomment-596465234
https://github.com/ipld/go-ipld-prime/pull/49#issuecomment-597549108
https://github.com/ipld/go-ipld-prime/pull/49#issuecomment-604412736

The "inlinability_test.go" file can be compiled with special flags
(described in the comment at the top of the file) to see the
outcome in assembly.  Result?  Yep, things are still inlinable;
this change is performance neutral.

Update to the hackme document to match.
(Turns out a decent amount of early speculations about how handy it
would be to nil out the 'w' pointer ended up a bit misguided.)

This spawns another one of those things that's dang hard to test:
half of these functions are for handling the case that something of
the right *kind* but a different *style*/implementation is given...
which we can't test except by ramming two different node
implementations together.  Even later, that's going to make things
rather Interesting, because the test package dependencies will
include one concrete node package and then be that much less usable
by that package itself (ugh); getting correct coverage attribution
to the odd package out there will be hard; and for now, it means we
just plain have no tests for those branches.  Making tests to check
compatibility and correctness of these branches once we get more
codegen integration up will be fun, though.

It needs this for dealing with the exciting details of...
well, you can read the comment.

This is a heck of an example of how the schema system, even though
we've been almost completely successful in isolating it in packages
and not letting it appear or be referenced in any of the core
interfaces and main dependencies... is still informing and shaping
a few key details of the central interfaces.

Future IPLD library implementers in other languages may want to take
note of this commit for the above reason: it's useful to make sure
you've also taken account of schema systems in your library design
at an early point.  Even if you don't intend to implement them before
doing a 1.0 of the core interfaces and Data Model essentials,
knowing what expressiveness will be needed will save you a lot of work.

Saying "Node" again in the middle is just purely redundant and doesn't
improve clarity in any meaningful way.  Begone with it.

With continuing thanks to @rvagg's good eyes: https://github.com/ipld/go-ipld-prime/pull/49#discussion_r389603173

Called out in https://github.com/ipld/go-ipld-prime/pull/49#discussion_r389604191 .

Per suggestion in https://github.com/ipld/go-ipld-prime/pull/49#discussion_r389606762 .

Votes for no in https://github.com/ipld/go-ipld-prime/pull/49#discussion_r389603606
and https://github.com/ipld/go-ipld-prime/pull/49#discussion_r389611401 .

This is a *lot* of changes.  It's the most significant change to date,
both in semantics and in character count, since the start of this repo.
It changes the most central interfaces, and significantly so.

But all tests pass.  And all benchmarks are *improved*.

The Node interface (the reading side) is mostly unchanged -- a lot of
consuming code will still compile and work just fine without changes --
but any other Node implementations out there might need some updating.

The NodeBuilder interface (the writing side) is *extremely* changed --
any implementations out there will *definitely* need change -- and most
consumers will too.  It's unavoidable with a semantic fix this big.
The performance improvements should make it worth your while, though.

If you want more background on how and why we got here, you've got
quite a few commits on the "research-admissions" branches to catch up
on reading.  But here's a rundown of the changes:

(Get a glass of water or something calming before reading...)

=== NodeAssembler introduced! ===

NodeAssembler is a new interface that describes most of the work of
creating and filling data into a new Node.

The NodeBuilder interface is still around, but changed in role.
A NodeBuilder is now always also a NodeAssembler; additionally, it can
return the final Node to you.

A NodeAssembler, unlike NodeBuilder, can **not** return a Node to you.
In this way, a NodeBuilder represents the ability to allocate memory.
A NodeAssembler often *does not*: it's just *filling in* memory.

This design overall is much more friendly to efficient operations:
in this model, we do allocations in bulk when a NodeBuilder is used,
and then NodeAssemblers are used thereafter to fill it in -- this
mental model is very friendly to amortizing memory allocations.
Previously, the NodeBuilder interface made such a pattern of use
somewhere between difficult and outright impossible, because it was
modeled around building small values, then creating a bigger value and
inserting the smaller ones into it.

This is the key change that cascaded into producing the entire other
set of changes which land in this commit.

The NodeBuilder methods for getting "child builders" are also gone
as a result of these changes.  The result feels a lot smoother.
(You can still ask for the NodeStyle for children of a recursive kind!
But you'll find that even though it's possible, it's rarely necessary.)

We see some direct improvements from this interface change already.
We'll see even more in the future: creating values when using codegen'd
implementations of Node was hugely encumbered by the old NodeBuilder
model; NodeAssembler *radically* raises the possible ceiling for
performance of codegen Node implementations.

=== NodeStyle introduced ===

NodeStyle is a new interface type that is used to carry information
about concrete node implementations.

You can always use a NodeStyle to get a NodeBuilder.

NodeStyle may also have additional features on it which can be detected
by interface checks.  (This isn't heavily used yet, but we imagine it
might become handy in the future.)

NodeStyle replaces NodeBuilder in many function arguments,
because often what we wanted was to communicate a selection of Node
implementation strategy, but not actually the start of construction;
the NodeStyle interface now allows us to *say that*.

NodeStyle typically cost nothing to pass around, whereas a NodeBuilder
generally requires an allocation to create and initialize.  This means
we can use NodeStyle more freely in many contexts.

=== node package paths changed ===

Node implementations are now in packages under the "node/*" directory.
Previously, they were under an "impl/*" directory.

The "impl/free" package is replaced by the the "node/basic" package!
The package name was "ipldfree"; it's now "basicnode".

=== basicnode is an improved runtime/anycontent Node implementation ===

The `basicnode` package works much the same as the `ipldfree` package
used to -- you can store any kind of data in it, and it just does as
best it can to represent and handle that, and it works without any
kind of type info nor needs of compile-time special support, etc --
while being just quietly *better at it*.

The resident memory size of most things has gone down.
(We're not using "fat unions" in the implementation anymore.)

The cost of iterating maps has gone down *dramatically*.
Iteration previously suffered from O(n) allocations due to
expensive `runtime.conv*` calls when yielding keys.
Iteration is now O(1) (!!) because we redesigned `basicnode` internals
to use "internal pointers" more heavily, and this avoids the costs
from `runtime.conv*`.
(We could've done this separately from the NodeAssembler change,
admittedly.  But both are the product of research into how impactful
clever use of "internal pointers" can be, and lots of code in the
neighborhood had to be rewritten for the NodeAssembler interface,
so, these diffs arrive as one.)

Error messages are more informative.

Many small operations should get a few nanoseconds faster.
(The implementation uses more concrete types and fewer switch
statements.  The difference probably isn't the most noticeable part of
all these changes, but it's there.)

--- basicnode constructor helpers do all return pointers ---

All the "New*" helper functions in the basicnode package return
interfaces which are filled by a pointer now.
This is change from how they worked previously when they were first
implemented in the "rsrch" package.

The experience of integrating basicnode with the tests in the traversal
package made it clear that having a mixture of pointer and non-pointer
values flying around will be irritating in practice.  And since it is
the case that when returning values from inside a larger structure,
we *must* end up returning a pointer, pointers are thus what we
standardize on.

(There was even some writeup in the HACKME file about how we *might*
encounter issues on this, and need to change to pointers-everywhere --
the "pointer-vs-value inhabitant consistency" heading.  Yep: we did.
And since this detail is now resolved, that doc section is dropped.)

This doesn't really make any difference to performance.
The old way would cause an alloc in those method via 'conv*' methods;
the new way just makes it more explicit and go through a different
runtime method at the bottom, but it's still the same number of
allocations for essentially the same reasons.  (I do wonder if at some
future point, the golang compiler might get cleverer about eliding
'conv*' calls, and then this change we make here might be unfortunate;
but that's certainly not true today, nor in the future at any proximity
that I can foresee.)

=== iterator getters return nil for wrong-kind ===

The Node.MapIterator and Node.ListIterator methods now return nil
if you call them on non-maps or non-lists.

Previously, they would return an iterator, but using it would just
constantly error.

I don't think anyone was honestly really checking those error thunks,
and they made a lot of boilerplate white noise in the implementations,
and the error is still entirely avoidable by checking the node kind
up-front (and this is strictly preferable anyway, since it's faster
than getting an error thunk, poking it to get the error, etc)...
so, in total, there seem like very few reasons these were useful:
the idea is thus dropped.

Docs in the Node interface reflect this.

=== node/mixins makes new Node implementations easier ===

The mixins package isn't usable directly, but if you're going to make
a new Node implementation, it should save you a lot of typing...
and also, boost consistency of basic error handling.

Codegen will look forward to using this.  (Codegen already had much of
these semantics internally, and so this package is sort of lifting that
back out to be more generally usable.  By making it live out here as
exported symbols in the core library, we should also reduce the sheer
character count of codegen output.)

=== 'typed.Node' is now 'schema.TypedNode' ===

A bunch of interfaces that were under the "impl/typed" path moved to
be in the "schema" package instead.  This probably makes sense to you
if you look at them and needs no further explanation.

(The reason it comes in this diff, though, is that it was forced:
adding better tests to the traversal package highlighted a bunch of
cyclic dependency issues that came from 'typed.Node' being in a
package that had concrete use of 'basicnode'.)

=== codecs ===

The 'encoding' package is now named 'codec'.

This name is shorter; it's more in line with vocabulary we use
elsewhere in the IPLD project (whereas 'encoding' was more of a nod
to the naming found in the golang standard library); and in my personal
opinion it does better at describing the both directions of the process
(whereas 'encoding' sounds like only the to-linear-bytes direction).

I just like it better.

=== unmarshal functions no longer return node ===

Unmarshal functions accept an NodeAssembler parameter (rather than
a NodeBuilder, as before, nor a NodeStyle, which might also make sense
in the new family of interfaces).

This means they no longer need to return a Node, either -- the caller
can decide where the unmarshalled data lands.  If the caller is using
a NodeBuilder, it means they can call Build on that to get the value.
(If it's a codegen NodeBuilder with More Information, the caller can
use any specialized functions to get the more informative pointers
without need for casting!)

Broadly speaking, this means users of unmarshal functions have more
control over how memory allocation comes into play.

We may want to add more helper functions to the various codec packages
which take a NodeStyle argument and do return a Node.  That's not in
this diff, though.  (Need to decide what pattern of naming these
various APIs would deserve, among other things.)

=== the fluent package ===

The fluent package changed significantly.

The readonly/Node side of it is dropped.  It didn't seem to get a ton
of exercise in practice; the 'traversal' package (and in the future,
perhaps also a 'cursor' package) addresses a lot of the same needs,
and what remains is also covered well these days by the 'must' package;
and the performance cost of fluent node wrappers as well as the
composability obstruction of them... is just too much to be worth it.

The few things that used fluent.Node for reading data now mostly use
the 'must' package instead (and look better for it, imo).

It's possible that some sort of fluent.Node will be rebuilt someday,
but it's not entirely clear to me what it should look like, and indeed
whether or not it's a good idea to have in the repo at all if the
performance of it is counterindicated in a majority of situations...
so, it's not part of today's update.

The writing/NodeBuilder/NodeAssembler fluent wrappers are continued.
It's similar to before (panics promptly on errors, and has a lot of
closures)... but also reflects all of the changes made in the migration
towards NodeAssembler: it doesn't return intermediate nodes, and
there's much less kerfuffle with getting child builders.
Overall, the fluent builders are now even more streamlined than before;
the closures need even fewer parameters; great success!

The fluent.NodeAssembler interface retains the "Create" terminology
around maps and lists, even though in the core interfaces,
the ipld.NodeAssembler interface now says "Begin" for maps and lists.
This is because the fluent.NodeAssembler approach, with its use of
closures, really does do the whole operation in one swoop.

(It's amusing to note that this change includes finally nuking some
fairly old "REVIEW" comment blocks from the old fluent package which
regarded the "knb" value and other such sadness around typed recursion.
Indeed, we've finally reviewed that: and the answer was indeed to do
something drastically different to make those recursions dance well.)

=== selectors ===

Selectors essentially didn't change as part of this diff.  Neat.

(They should get a lot faster when applied, because our node
implementations hit a lot less interface boxing in common operations!
But the selector code itself didn't need to change to get the gains.)

The 'selector/builder' helper package *did* change a bit.
The changes are mostly invisible to the user.
I do have some questions about the performance of the result; I've got
a sneaking suspicion there's now a bunch of improvements that might be
easier to get to now than they would've been previously.  But, this is
not my quest today.  Perhaps it will deserve some review in the future.

The 'selector/builder' package should be noted as having some
interesting error handling strategies.  Namely, it doesn't.
Any panics raised by the fluent package will just keep rising; there's
no place where they're converted to regular error value returns.
I'm not sure this is a good interface, but it's the way it was before
I started passing through, so that's the way it stays after this patch.

ExploreFieldsSpecBuilder.Delete disappears.  I hope no one misses it.
I don't think anyone will.  I suspect it was there only because the
ipld.MapBuilder interface had such a method and it seemed like a
reasonable conservative choice at the time to proxy it; now that the
method proxied is gone, though, so too shall go this.

=== traversal ===

Traversal is mostly the same, but a few pieces of config have new names.

`traversal.Config.LinkNodeBuilderChooser` is now
`traversal.Config.LinkTargetNodeStyleChooser`.
Still a mouthful; slightly more accurate; and reflects that it now
works in terms of NodeStyle, which gives us a little more finesse in
reasoning about where NodeBuilders are actually created, and thus
better control and insight into where allocations happen.

`traversal.NodeBuilderChooser` is now
`traversal.LinkTargetNodeStyleChooser` for the same reasons.

The actual type of the `LinkTargetNodeStyleChooser` now requires
returning a `NodeStyle`, in case all the naming hasn't made it obvious.

=== disappearing node packages ===

A couple of packages under 'impl/*' are just dropped.

This is no real loss.  The packages dropped were Node implementations
that simply weren't done.  Deleting them is an increase in honesty.

This doesn't mean something with the same intentions as those packages
won't come back; it's just not today.

--- runtime typed node wrapper disappeared ---

This one will come back.  It was just too much of a pain to carry
along in this diff.  Since it was also a fairly unfinished
proof-of-concept with no downstream users, it's easier to drop and
later reincarnate it than it is to carry it along now.

=== linking ===

Link.Load now takes a `NodeAssembler` parameter instead of a
`NodeBuilder`, and no longer returns a `Node`!

This should result in callers having a little more control over where
allocations may occur, letting them potentially reuse builders, etc.

This change should also make sense considering how codec.Unmarshal
now similarly takes a NodeAssembler argument and does not return
a Node value since its understood that the caller has some way to
access or gather the effects, and it's none of our business.

Something about the Link interface still feels a bit contorted.
Having to give the Load method a Loader that takes half the same
arguments all over again is definitely odd.  And it's tempting to take
a peek at fixing this, since the method is getting a signature change.
It's unclear what exactly to do about this, though, and probably
a consequential design decision space... so it shall not be reopened
today during this other large refactor.  Maybe soon.  Maybe.

=== the dag-json codec ===

The dag-json codec got harder to implement.  Rrgh.

Since we can't tell if something is going to become a Link until
*several tokens in*, dag-json is always a bit annoying to deal with.
Previously, however, dag-json could still start optimistically building
a map node, and then just... quietly drop it if we turn out to be
dealing with a link instead.  *That's no longer possible*: the process
of using NodeAssembler doesn't have a general purpose mechanism for
backtracking.

So.  Now the dag-json codec has to do even more custom work to buffer
tokens until it knows what to do with them.  Yey.

The upside is: of course, the result is actually faster, and does fewer
memory allocations, since it gathers enough information to decide what
it's doing before it begins to do it.
(This is a lovely example of the disciplined design of NodeAssembler's
interface forcing other code to be better behaved and disciplined!)

=== traversal is faster ===

The `BenchmarkSpec_Walk_MapNStrMap3StrInt/n=32` test has about doubled
in speed on the new `basicnode` implementation in comparison to the old
`ipldfree.Node` implementation.

This is derived primarily from the drop in costs of iteration on
`basicnode` compared to the old `ipldfree.Node` implementation.

Some back-of-the-envelope math on the allocation still left around
suggest it could double in speed again.  The next thing to target
would be allocations of paths, followed by iterator allocations.
Both are a tad trickier, though (see a recently merge-ignore'd
commit for notes on iterators; and paths... paths will be a doozy
because the path forward almost certainly involves path values
becoming invalid if retained beyond a scope, which is... unsafe),
so certainly need their own efforts and separate commits.

=== marshalling is faster ===

Marshalling is much faster on the new `basicnode` implementation in
comparison to the old `ipldfree.Node` implementation.
Same reasons as traversal.

Some fixes to marshalling which previously caused unnecessary
allocations of token objects during recursions have also been made.
These improve speed a bit (though it's not nearly as noticeable as the
boost provided by the Node implementation improvements to iteration).

=== size hints showed up all over the place ===

The appearance of size hint arguments to assembly of maps and lists
is of course inevitable from the new NodeAssembler interface.

It's particularly interesting to see how many of them showed up in
the selector and selectorbuilder packages as constants.

And super especially interesting how many of them are very small
constants.  44 zeros.  86 ones.  25 twos.  9 threes.  2 fours.
(Counted via variations of `grep -r 'Map(.*4, func' | wc -l`.)
It's quite a distribution, neh?  We should probably consider some
more optimizations specifically targeted to small maps.
(This is an unscientific sample, and shifted by what we chose to
focus on in testing, etc etc, but the general point stands.)

`-1` is used to indicate "no idea" for size.  There's a small fix
to the basicnode implementations to allow this.  A zero would work
just as well in practice, but using a negative number as a hint to
the human seems potentially useful.  It's a shame we can't make the
argument optional; oh well.

=== codegen ===

The codegen packages still all compile... but do nonsensical things,
for the moment: they've not been updated to emit NodeAssembler.

Since the output of codegen still isn't well rigged to test harnesses,
this breakage is silent.

The codegen packages will probably undergo a fairly tabula-rasa sweep
in the near future.  There's been a lot of lessons learned since the
start of the code currently there.  Updating to emit the NodeAssembler
interface will be such a large endeavor it probably represents a good
point to just do a fresh pass on the whole thing all at once.

--------

... and that's all!

Fun reading, eh?

Please do forgive the refactors necessary for all this.  Truly, the
performance improvements should make it all worth your while.

See comments in that commit.  It's a good idea, but needs some further
refinement in order to really shine.

It's possible, but this is a bit less trivial than it sounds...
so I think this is one of those diffs I'm going to *save*, but
via a "merge-ignore", and come back to the topic again later.

Here's where we were at before this diff:

```
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=0-8    3882726               308 ns/op             448 B/op          5 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=1-8    1273840               961 ns/op             464 B/op          6 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=2-8     690142              1785 ns/op             624 B/op          8 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=4-8     376281              3197 ns/op             944 B/op         11 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=8-8     197530              6084 ns/op            1584 B/op         16 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=16-8    102693             11534 ns/op            2864 B/op         25 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=32-8     54110             22601 ns/op            5424 B/op         42 allocs/op
```

(This is still a very sizable improvement over the node implementations on master;
about double the speed and a quarter of the allocs.  But still, could be improved?)

With this diff adding iterator amortization (and a bench loop with timer pauses in it):

```
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=0-8            1580808               778 ns/op             432 B/op          4 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=1-8             744278              1574 ns/op             432 B/op          4 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=2-8             468837              2529 ns/op             576 B/op          5 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=4-8             277303              4171 ns/op             864 B/op          6 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=8-8             172590              7187 ns/op            1440 B/op          7 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=16-8             93064             12494 ns/op            2592 B/op          8 allocs/op
BenchmarkSpec_Marshal_MapNStrMap3StrInt/n=32-8             52755             22671 ns/op            4896 B/op          9 allocs/op
```

So... this is kind of an unexpected result.  We got the allocs to go down,
as intended.  But speed didn't go up.  What gives?

A priori, I've been running with the ballpark number of "88ns per alloc"
(derived from some other observations on these packages today), which...
would lead me to compute an expected gain in speed of about `88*(42-9)`
given how many fewer allocations we see here... which in turn would lead
to an expectation of about 12.8% speed improvement.
Which... we don't see at all.  The speed is more or less unchanged.
Hrm.

I suspect that the reason for this is that having StartTimer/StopTimer
calls in the middle of your `b.N` loop just is not possible to do without
having serious skewing effects on results, and if we could clear away
the skew, we'd probably see the expected improvement.

But still, in light of this unclarity, I'm going to merge-ignore this.
We can come back to this optimization later.
After this reflection, I feel ~12% isn't worth chasing today,
especially if it's a complicated 12%, and especially since we already
have other much larger effects that are still in progress of getting
properly landed and into use on the master branch.

Another issue that deserves due consideration is how this change would
interact with concurrent use of datastructures.  Since nodes are
immutable, we *should* usually be able to expect that they can be read
across threads without issue.  This patch would break that: getting
two different iterators could be a race condition.  Fixing this is easy
(I'd use https://golang.org/pkg/sync/atomic/#CompareAndSwapUint32),
but raises the scope just a bit, and files this further as "not today".

Iterators might also deserve a "Close" method in order to make this
kind of optimization more accessible to the user; there's no reason it
has to be once-only.  (This would also just so happen to fix the reason
we needed StartTimer/StopTimer in the b.N loop, making measurement of
the improvement easier.)

Lots of things to consider when tackling this properly in the future!

Sizable performance impact.

Recently added scalable benchmarks helped point this out.

(Arguably, certainly, I could've/should've seen this one earlier; it's
honestly pretty "duh".  But somehow it never got priority attention.
A benchmark is still awfully useful for directing attention!)

With this, the new node marshal is looking *really* snazzy.
Most of the allocations remaining there are just iterator creation...
and we have a plan to amortize those out of existence, too.
But I'm going to save that one for later.

Marshal is 25% faster.  Not bad.  (There's also a perf bug in the codec
code which I'm about to fix which will change the speedup to *double*.)

Unmarshal is a bit faster, but not as much improved as marshal.
This is unsurprisingly because we chose not to amortize allocations
as much as we could in map and list creation because the size penalty
seemed too steep to make it a worthy trade.  (This gives us *much*
better numbers to fuel further consideration of that trade, though.)

Dear reader, I am so excited about this.

Also: added additional sanity checks to the end of benchmarks, after
a call to `b.StopTimer`.  Always important to make sure your benchmark
is actually doing what you think it is, and not being super fast
because it turns out to be a hyperspeed application-layer devnull!

(In the case of the unmarshal tests, asserting sanity by remarshalling
the whole dang thing *again* might look a little funny... but it's
certainly a terse way to get full coverage of the data!)

This is one of the key goals I've been trying to work toward with all
the fixture nomenclature efforts.  (Correspondingly, finally this is
a naming pattern I'm satisfied with, as well.)

Here's an example output, from the ipldfree package:

```
BenchmarkSpec_Walk_Map3StrInt-8                          1501033               802 ns/op             624 B/op          9 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=0-8               6288885               191 ns/op              96 B/op          3 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=1-8               1000000              1160 ns/op             896 B/op         13 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=2-8                520072              2326 ns/op            1696 B/op         23 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=4-8                239588              4293 ns/op            3296 B/op         43 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=8-8                138903              8461 ns/op            6496 B/op         83 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=16-8                70845             16951 ns/op           12896 B/op        163 allocs/op
BenchmarkSpec_Walk_MapNStrMap3StrInt/n=32-8                33601             33793 ns/op           25696 B/op        323 allocs/op
```

This roughly linear increases in time taken and in allocs required as
the size of the data we walk is increased.

(What I'm hoping to see when we land the NodeAssembler refactor and
other key improvements to reduce interface boxing costs that were
co-developed with that change is: we'll see the allocs become O(1),
and the linearity on the time will become a much less sharp incline!)

I suspect one could parse these names back apart to draw graphs for
the various sizes.  (I think such tools exist out there already, though
I don't have a working knowledge of them yet.  If something suitable
doesn't exist already, the basic pieces to assemble something do:
https://github.com/golang/perf/blob/36b577b0eb03b831f9f591c1338a115cafcb56a7/storage/benchfmt/benchfmt.go#L31
has a parser for the output format.)

We should also be able to reuse these benchmarks for other Node
implementations fairly effortlessly.  I'm particularly looking forward
to seeing how that shapes up when we apply it to codegen'd stuff.

In the future, I want to rack up even more data in name patterns like:

 baselines.BenchmarkSpec_Unmarshal_MapStrInt/codec=json/n=3
 baselines.BenchmarkSpec_Unmarshal_MapStrInt/codec=json/n=25
 basicnode.BenchmarkSpec_Unmarshal_MapStrInt/codec=json/n=3
 basicnode.BenchmarkSpec_Unmarshal_MapStrInt/codec=json/n=25
 basicnode.BenchmarkSpec_Unmarshal_MapStrInt/codec=cbor/n=3
 basicnode.BenchmarkSpec_Unmarshal_MapStrInt/codec=cbor/n=25
   gendemo.BenchmarkSpec_Unmarshal_MapStrInt/codec=json/n=3
   gendemo.BenchmarkSpec_Unmarshal_MapStrInt/codec=json/n=25
   gendemo.BenchmarkSpec_Unmarshal_MapStrInt/codec=cbor/n=3
   gendemo.BenchmarkSpec_Unmarshal_MapStrInt/codec=cbor/n=25

The general pattern being:

  {nodeImplementation}.BenchmarkSpec_{Application}_{FixtureCohort}/codec={codec}/size={size}

Some variations like "codec=" will only exist for some applications,
e.g. it does exist for marshal and unmarshal, but of course doesn't
apply for traversal.

In the process, noticed we're using the test subject nodebuilder for
part of the selector setup too... which is... well, it's not wrong,
per se, but it's certainly tangental.  I don't see any good way to
avoid it though, so, there's now just a comment about this fact.

Lots of import cycle fixing was necessary to get here, which was an
unpleasant surprise at the start of the day... but lots of things are
better off now, so it was highly worth it.

This is the last part of the cleanup and cyclic import fix started
in the previous commit.

Previously it was in the 'impl/typed' package, next to the
runtime-wrapper implementation of the interface.  This was strange.

Not only should those two things be separated just on principle,
this was also causing more import cycle problems down the road:
for example, the traversal package needs to consider the *interface*
for a schema-typed node in order to gracefully handle some features...
and if this also brings in a *concrete* dependency on the
runtime-wrapper implementation of typed nodes, not only is that
incorrect bloat, it becomes a show stopper because (currently, at
least) that implementation also in turn transitively imports the
ipldfree package for some of its scalars.  Ouchouch.

So.  Now the interface lives over in the 'schema' package, with all
the other interfaces for that feature set.  Where it probably always
should have been.

('typed.Maybe' also became known as 'schema.Maybe', which... does not
roll off the tongue as nicely.  But this is a minor concern and we
might reconsider the naming and appearance of that thing later anyway.)

traversal.NodeBuilderChooser can now return error.  Also, the default no longer returns ipldfree.NodeBuilder.

The traversal package having a dependency on the ipldfree Node
implementation package was problematic: it's important that we be able
to benchmark traversal *in combination* with different implementations
of Node, and it turns out if we want to build reusable test and
benchmark functions for that, we get cyclic dependencies when then
trying to use them on our most common implementation!  Ouch.

Having a default implementation of Node referenced was originally out
of a desire for convenience.  But it's logically dubious anyway.
Even the convenience gain in practice is questionable since one still
always needs to configure a LinkLoader in the same areas.  At any rate,
hopefully the error message is helpful enough to make this low-impact.

(We could probably benefit from more helper methods around this, too.
But we can review that later.  Preferably after the 'NodeStyle' feature
from the R&D branches lands, also, which should make this whole area
better named, clearer about where its allocations arrive, and just
less fidgety in general.)

Getting *enough* and sufficiently *organized* corpuses becomes a
legitimate challenge.

The docs outline some of the directions this will go while describing
the naming convention.

This naming convention has already been cropping up in an ad-hoc way in
recent commits; this is a step towards documenting it consistently.

There aren't many entries yet; expect it to grow.

Using JSON as the defacto format is a little aggressive, perhaps,
because it makes sort of a wide dependency span.
But since we've already long had unmarshalling of json working,
it seems viable in practice.
And it means we get the marshalling output target corpuses for free
for at least one of our formats.
And it means we can readily make comparisons to stdlib json,
which is nice for having baselines to frame comparisons against.
It also has the interesting sideeffect of making these corpuses
immune to change in the face of refactors to NodeBuilder (which
will be an absurd concern at any time except... right now).
(We'll see.  Maybe I'll regret this after some time passes.  But if so,
this content probably just pivots to being still useful in json
marshal and unmarshal tests.)

I'd like to put this to work in writing more traversal benchmarks...
but that's going to have to wait a few commits, because I've found some
import cycles that get very problematic when I try to proceed there,
and it looks like they might take a few steps to sort out.