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

[warpfork]

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.

To build a map with the NodeAssembler design, you would first begin
the map, receiving a MapNodeAssembler; then make a sequence of
AssembleKey and AssembleValue calls, both of which give you
another NodeAssembler you can operate recursively on; and at the
end you call Finish.

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 ordering left no possibility
for arranging things to support amortization of allocations.

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.)

In most places where you previously used ipldfree.NewBuilder,
you might now expect to use basicnode.Style__Any{}.
You can also use one of the other Style__* values if you know
you're going to be creating values of a particular kind
(e.g., you can use Style__Map if you know you're going to create
map values; but if you get a builder from this and try to create
a list with it, you'll then get an error).

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.
(From the point of view of the master branch, this isn't a change at
all, because the whole package is new -- but it is change from how they
worked when they were first implemented in the "rsrch" package, and
it's a change I made during the integration mega-commit, so it deserves
a brief note of mention.)

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,
and the overall system getting faster as a result!)

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.

[codecov]

Codecov Report

Merging #49 into master will decrease coverage by 21.13%.
The diff coverage is 35.78%.

@@             Coverage Diff             @@
##           master      #49       +/-   ##
===========================================
- Coverage   72.28%   51.15%   -21.12%     
===========================================
  Files          52       67       +15     
  Lines        3217     4751     +1534     
===========================================
+ Hits         2325     2430      +105     
- Misses        798     2222     +1424     
- Partials       94       99        +5     

Impacted Files	Coverage Δ
codec/dagjson/marshal.go	31.20% <ø> (ø)
fluent/fluentRecover.go	85.72% <0.00%> (-14.28%)	⬇️
node/basic/bool.go	0.00% <0.00%> (ø)
node/basic/bytes.go	0.00% <0.00%> (ø)
node/basic/float.go	0.00% <0.00%> (ø)
node/basic/int.go	2.78% <0.00%> (ø)
node/basic/link.go	0.00% <0.00%> (ø)
node/basic/list.go	0.00% <0.00%> (ø)
node/gendemo/int.go	0.00% <0.00%> (ø)
node/gendemo/map_K2_T2.go	0.00% <0.00%> (ø)
... and 50 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb71617...06aa20a. Read the comment docs.

[warpfork]

Couple things still needed here:

• AssembleDirectly should be renamed something less nonsensical. (Suggestions welcome.)
• I suspect s/{Map,List}NodeAssembler/{Map,List}Assembler/g would be an improvement to terseness with no loss of legibility.
• ListAssembler.ValueStyle() should take an index int param! (This detail matters only for typed nodes with struct kind, also as the docs say, ValueStyle basically isn't exercised at all, but it still ought to be right.)
• AssignNode on a bunch of types needs to be fully implemented (namely, basicnode.plainMap__Assembler and basicnode.plainList__Assembler). Ideally via a generic pure Copy func that will work over any Node implementation. (There wasn't really an equivalent to this in the old interface, so nothing exercises it yet, but it still ought to be right.)

Technically this commit could be merged as-is and we could start propagating updates to downstreams, because these are all either trivial sed-refactor things, or currently unexercised feature paths; but it's probably still best to wrap these changes up sooner than later.

[warpfork]

Side note, I really don't know if I think much of this code coverage bot we've got rigged up here. On the rare occasion I click through to it... it's showing node/basic/map.go as a big red baddie? What? That's completely wrong; that's one of the most intensely covered files.

I don't have any idea what is going on over in that bot's head, but I can't say I'm impressed by the quality of its visualizations, nor am I particularly inclined to give it attention or take it seriously.

[rvagg]

nor am I particularly inclined to give it attention or take it seriously

RIP IT OUT

that's a clear sign that the tool shouldn't be there, bring it (or something else) back when you find a workflow that you have confidence in.

[rvagg]

Does ShouldEqual have access to enough of the internals to make a meaningful comparison between two Nodes?

[rvagg]

Intermediate thought while reading code: Style__* reads like codegen; what kind of alternatives do we have available for naming these? I'd love a Style.Map but I don't think we can really do that in Go can we? StyleMap would probably be fine but I think I understand why you want to separate the two words more.
(Still not a superfan of "Style")

Edit

Seeing Style__String{} next to ReprKind_String makes this stand out even more, a discrepancy wanting to be addressed somehow.

[rvagg]

I really like the pattern of codecs Decoder() calls getting passed the NodeAssembler rather than returning a Node. Makes for some interesting extension possibilities. I'm definitely going to steal that one day in JS.

[rvagg]

yeah, don't make ErrWrongKind's name longer, it's nice as is

[warpfork]

Style__* reads like codegen; what kind of alternatives do we have available for naming these? I'd love a Style.Map but I don't think we can really do that in Go can we?

It does read like codegen. And the same issue will show up there, but with even more names in the star match, which might be interesting for how we decide to handle it.

I think we can lunge for basicnode.Style.Map, actually. Make a single struct with each of the nodestyle types embedded in it; make one of them and assign it to a package scope variable.

I'm glad you brought this up, because I'd already been thinking about it, but unsure if it's worth doing something that's arguably idiomatically Odd for this. Sounds like you'd vote "yes"?

I've been trying to think of any other downsides (or other alternatives) to the pkgvar-of-struct-of-structs idea, but I think I'm coming up pretty dry. It will tend to make references to basicnode.Style.Map require memory dereference, and make the subsequent function calls on it uninlinable, I'm afraid, because technically that pkgvar will be mutable. But maybe that's inconsequential in practice. (And maybe, actually, ideally, the go compiler will still be smart enough to inline things anyway -- it should be able to see that the methods on the type that basicnode.Style.Map must be (it would still be a basicnode.style__map, probably, or whatever; just unexported now, because no need to export it) will be methods on an empty (zero-cardinality) struct, and it should be able to get clever with that.)

... okay, was thinking out loud a bit in that last paragraph; I think I've talked myself into needing to write a test of this.

[warpfork]

Seeing Style__String{} next to ReprKind_String makes this stand out even more, a discrepancy wanting to be addressed somehow.

I use dunder (double-underscore, if you will) to say "danger, codegen was here" (and because I don't think ruling out single underscores in type names in schemas is reasonable).

I use single underscores in regular code to separate enum type names from member names. (In other languages, this wouldn't be necessary, but Go Doesn't Really Have Enums.)

So I hope we can make the "__" instances disappear from view entirely, as discussed in the above comment. Otherwise I'm afraid I'll be inclined to keep this, because I think using double-underscore as a default collision-avoidance mechanism in codegen is still advisable, and it would also be nice if the entrypoints to codegen usage look roughly the same as the builtin basicnode stuff.

[hannahhoward]

@warpfork FWIW, even in it's very early/not updated state, I find the codegen super useful. What is the timeline for updating it to at least work with the new interfaces?

[rvagg]

still reviewing this (although I don't want to be a holdup if this is close to landing), going to post these notes I wrote last week and don't want to lose if my browser needs a restart:

• The level of complication in dagjson/unmarshal.go doesn't seem to be matched with the amount of testing it receives
• Suggestion for renaming AssembleDirectly: AssembleEntry

[warpfork]

even in it's very early/not updated state, I find the codegen super useful. What is the timeline for updating it to at least work with the new interfaces?

Well, it's now bumped up to about max priority, since you're asking :P

The level of complication in dagjson/unmarshal.go doesn't seem to be matched with the amount of testing it receives

I think they're more passably tested than they look, but the clarity of this is not high. The cbor and json encoding from tokens onward is covered by tests the refmt repo. The majority of the node traversing tests are in the node/tests/* package and then applied per node implementation package. The marshal and unmarshal code is just the tiny bits of glue left between the two... so they have some smoke tests that make sure recursion works right, and presume fairly little is likely to go wrong outside of that, because one of the other two flanking test suites should've caught it.

More would still be better, of course.

Suggestion for renaming AssembleDirectly: AssembleEntry

Better. Applying it now. Thank you.

Style__Map{} vs Style.Map

I still want to do a little research on this, but I'm tending towards wanting to go for it. I'm probably going to kick that out into a future PR though, because I'm starting to feel inclined to land this so any other work starting in parallel gets the biggest changes in under the belt. (Refactoring s/Style__Map{}/Style.Map/ at a later date will literally be a sed-refactor, unlike any code that gets written against today-master and then has to be updated to account for all the fun in this PR.)

[warpfork]

(Changed my mind about kicking the "Style" syntax out until later.)

It will tend to make references to basicnode.Style.Map require memory dereference, and make the subsequent function calls on it uninlinable, I'm afraid, because technically that pkgvar will be mutable. [...] maybe, actually, ideally, the go compiler will still be smart enough to inline things anyway -- it should be able to see that the methods on the type that basicnode.Style.Map must [...] be methods on an empty (zero-cardinality) struct, and it should be able to get clever with that.

Did this test. Yes: the go compiler does get clever with that, and does manage to inline things. The phrase basicnode.Style__String{}.NewBuilder() and basicnode.Style.String.NewBuilder() will result result in identical inlining in whatever function uses them.

That removes any performance concern for the fancier syntax, and that was my only real potential objection at all... so let's go for it. Dunder-be-gone.

[warpfork]

This appears to be it. All the checkmarks from earlier todo lists are checked, and even a few of the syntax improvement suggestions made it in.

Time to merge.

(I tagged a 'v0.0.2' on master before this, in case anyone reading this doesn't want to absorb these changes yet, and needs a pointer to where that is.)