1. 11 Mar, 2020 2 commits
    • Eric Myhre's avatar
      ListAssembler.ValueStyle does need a parameter. · b3c369ae
      Eric Myhre authored
      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.
      b3c369ae
    • Eric Myhre's avatar
      Shorten: s/{Map,List}NodeAssembler/{Map,List}Assembler/g · 31cbbf82
      Eric Myhre authored
      Saying "Node" again in the middle is just purely redundant and doesn't
      improve clarity in any meaningful way.  Begone with it.
      31cbbf82
  2. 02 Mar, 2020 1 commit
    • Eric Myhre's avatar
      Promote NodeAssembler/NodeStyle solution to core. · 4eb8c55c
      Eric Myhre authored
      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.
      4eb8c55c
  3. 29 Feb, 2020 1 commit
    • Eric Myhre's avatar
      Attempt to amortize getting of an iterator. · 7c595d69
      Eric Myhre authored
      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!
      7c595d69
  4. 23 Feb, 2020 1 commit
    • Eric Myhre's avatar
      basicnode's constructors shall all prefix 'New'. · 29c1b98e
      Eric Myhre authored
      This first crossed my mind when considering whether it might be a good
      idea to export the concrete implementation types of the 'basicnode'
      package (see previous couple of commits -- it mostly had to do with the
      question of whether we could reuse more code and shrink the size of
      packages emitted by codegen)... but even though that's resolved to a
      pretty solid "nope", it still makes sense to have a consistent name
      prefix pattern here... and we'll probably encourage and expect to see
      similar conventions in other packages (including in codegen, if we
      add a handy-constructor-funcs feature to that).
      29c1b98e
  5. 20 Feb, 2020 1 commit
    • Eric Myhre's avatar
      Mega reorg; split prototype packages; much closer to ready to merge coreward as a result. · 9523d918
      Eric Myhre authored
      Turns out there was a lot of tech debt already accumulating very very
      rapidly due to the earlier whimiscal choice to lump the new basicnode
      implementations and the demo-codegen implementations into the same
      packages as benchmarking and research on this branch began.
      
      The basicnode package is now extracted cleanly, and exists in the
      'node/basic' path.  This 'basicnode' package is the equivalent of what
      was formerly called 'ipldfree' on the master branch.
      
      The demo codegen content is now under 'node/gendemo'.
      Note that it is BROKEN in this commit; there was too much there to
      clean up and address in one commit, and this one has already become
      quite large and includes a bewildering number of moves as well as
      finer content rearrangements.  Fixes will come in next commit(s).
      
      (Why?  Well, the demo codegen content leaned on the basicnode types
      for string and int.  Turns out this actually opens *quite* the
      can of worms when you try to rectify it.
      At the surface level, the basicnode package's types aren't exported.
      Could we export them?  Well, sure.  Except that's only half the trick.
      Can we export the *assemblers* for those types?  Well, sure.  But...
      Can we export the 'w' fields for those assemblers for those types?
      **NO** -- this is where it all breaks down -- if those fields are
      exported, we lose all control over all immutability everywhere.
      Shoot...?  Very shoot.
      One clearly correct solution to this is to generate new types for
      all of the primitive scalars in every codegen output package;
      the downside, obviously, is that's a bit verbose in resulting code.
      There might be another very alexandrian solution to this gordian knot:
      exporting the basicnode package's types after all, but not their
      assemblers; then leaning further into their implementation detail
      of being implemented as typedefs rather than strictly enclosed structs,
      and having every part of codegen types that handle primitive leafs
      directly perform and manage the relevant casts.  The bigger picture
      wisdom of this has not been completely analyzed yet.)
      
      The 'impls' path has in general become 'node'.
      This will stay this way when we finally merge over core on master;
      all the current 'impl' (without the s; sigh) paths will become 'node'.
      
      Numerous new HACKME files have appeared; some are splits of old ones
      to account for the new package splits; some are hoisted and polished
      from what were previously commentstorms in the code; some are
      completely new.  There are also a few new package-scope godocs.
      
      The benchmarks and tests are rearranged and much (much) improved.
      Naming is more consistent; and things are consistently extracted
      to the 'node/mixins/tests' package, and imported and used from there.
      (If you want all the same benchmark info as before, you'll now have
      to run benchmarks from several different packages.)
      9523d918
  6. 10 Feb, 2020 1 commit
    • Eric Myhre's avatar
      Address varying ValueStyle for structs. · dfd6c013
      Eric Myhre authored
      This is a rather important detail to remember, and one that is easy to
      lose track of if not actively wrangling the schema typed usecases.
      (And so, I almost did.  Uufdah.)
      
      The various methods for getting NodeStyle out of assemblers are now
      much better documented.
      
      As my current goals are still revolving around finishing the new
      'ipldfree'/'basic' nodes, wrapping up this whole research branch,
      and bringing these interface back to land in the library core...
      I'm still not bothering to substantially handle the inside of the
      ValueStyle methods on the example struct types.  But it should be
      clear enough how to implement them correctly now.
      dfd6c013
  7. 06 Feb, 2020 4 commits
  8. 05 Feb, 2020 6 commits
    • Eric Myhre's avatar
      Stamp out all the other scalars. · 9fda6b77
      Eric Myhre authored
      And some misc fixes in others (inconsistent method ordering where I
      updated existing files, mostly).
      
      Lists still coming up; being a recursive kind, those have much more
      involved code.
      9fda6b77
    • Eric Myhre's avatar
      AssignLink reinstated in NodeAssembler. · 252cfea5
      Eric Myhre authored
      (Probably should've done this a while ago.  Much earlier in this
      research branch, I thought the design of Link/LinkBuilder etc might
      get a review... but at this point, its clear there's plenty of
      work already do to just sorting out the assembler situation, so
      any rethinks of links is *definitely* deferred for future investigation
      (if indeed anything ever happens there at all).)
      
      Doing very partial fixes to the example "generated" maps;
      just enough to compile.  I'm not intending to use them as a template
      for actual codegen later, so their quality is irrelevant at this point.
      252cfea5
    • Eric Myhre's avatar
      plainMap__KeyAssembler should use string mixin. · b98d1ef0
      Eric Myhre authored
      It's clearly a string in all behavioral ways.
      
      I do pause to wonder if a different "TypeName" should be used here to
      provide more indication of context.  I don't have a strong argument
      one way or another.  Leaning against it by default, since so far when
      I've had questions about "should I make child assemblers announce
      their specialness" it has usually seemed to shake out to "no".
      b98d1ef0
    • Eric Myhre's avatar
      Mixins for assemblers. · 66e36be2
      Eric Myhre authored
      This removes all the shitty placeholder `panic("no")` calls which
      previously occupied this area, and gets us one step closer to being at
      the quality level where we can merge this into core.
      
      Also, a short template file to make it easier to stamp out the rest of
      these for the remaining kinds.  No automation; not worth it.
      66e36be2
    • Eric Myhre's avatar
      s/Assign/AssignNode/. · 64d1440b
      Eric Myhre authored
      As has been the priority in other cases, the shorter name should be
      reserved for codegen outputs, as those are where we are optimizing for
      ergonomics the most.
      64d1440b
    • Eric Myhre's avatar
      Introduce mixins, use for common kind features. · 1ac23897
      Eric Myhre authored
      They don't reduce line count, but they do make lines much shorter,
      and increase consistency, which are worth it.
      
      There's some very similar things already in the codegen system,
      though under a different name ("kindedRejectionHelper" something).
      I've realized the utility also exists beyond codegen.
      
      Codegen can in the future be updated to use these (and in the process,
      emit noticably fewer bytes of generated code).
      
      (There could be concerns about codegen output getting overly fragile
      and dependent on core library versions by reusing more code like this.
      However, I don't think that applies here: It's already going to be
      fragile if any of the actual error types change; being fragile if the
      mixins (which are *almost* entirely about those) change is effectively
      the same thing, it just saves a lot of output size.)
      
      I would collapse all these uninteresting methods into one line each
      rather than three lines each if I could... however, the golang
      formatter's (rather incogruous?) rule about breaking long lines in
      this *particular* case strikes.  Turns out in this corpus, for example,
      the `plainInt.LookupSegment` declaration *just* crosses over the
      length which forces a break, even though none of its siblings do.
      Sigh.
      1ac23897
  9. 03 Feb, 2020 7 commits
    • Eric Myhre's avatar
      Update defn of iterator accessors: return nil. · b25dc49d
      Eric Myhre authored
      The previously specified behavior has turned out to be *remarkably*
      annoying; much more so than I initially expected.  It's both not
      particularly pleasant to handle as a user; and even more unexpectedly,
      has turned out absolutely infuriating as a library author.
      (You can see how the early work on these new packages has simply
      resorted to a placeholder panic there, because the reject-carrying
      thunks cost an irritating amount of unimportant work.)
      
      The mapIteratorReject and listIteratorReject thunks have replicated
      too many times already, and this count is indicative of a problem:
      I'm not replicating them again into this new generation of interfaces.
      Nope.  Nuked.
      b25dc49d
    • Eric Myhre's avatar
      9edc9a08
    • Eric Myhre's avatar
      Clean up commentary. · b246be1b
      Eric Myhre authored
      b246be1b
    • Eric Myhre's avatar
      Map key and value assemblers invalidate self more aggressively when finished. · 0301dbad
      Eric Myhre authored
      This prevents later mutation by holding onto an assembler too long.
      (*Getting* the child assemblers is fenced by the 'state' field;
      but since none of the child assembler methods check the 'state',
      some defense is needed there.  Invalidating the pointer back up is it:
      this invalidation overhold of child assemblers once the user already
      has them into a nonissue by making any mutations fail.)
      
      I did a few extra-long benchmark runs to make sure this extra footwork
      doesn't cost any noticable time.  It does not; it's within the margins
      of error on a benchtime=4m run.
      0301dbad
    • Eric Myhre's avatar
      NodeAssembler.Done -> NodeAssembler.Finish. · f92f20eb
      Eric Myhre authored
      We use the word "done" as the predicate in iterators; therefore it's
      confusing to also use it as a verb for *doing* completion elsewhere.
      
      "Finish" also has the advantage of sounding slightly more active --
      which it is, since it's often involved in housekeeping and, well,
      *finishing* assignment within the enclosing parent value, as well as
      in evaluating validations for types that have them.
      f92f20eb
    • Eric Myhre's avatar
      Some effort to reduce cost of indirections needed for nested map assembly. · 00db3ff6
      Eric Myhre authored
      The original goal of insuring there's minimal speed costs turned out to
      be a red herring because it was already doing pretty reasonable things;
      but instead, the quest generated a little learning about assembly size.
      
      Comments about that continue to accumulate in the
      currently-backburner'd map+structs codegen file, where they are most
      relevant because of the question of whether to handle struct fields
      with generation of copious small types.
      00db3ff6
    • Eric Myhre's avatar
      Dedup assignment code in untyped map values. · 24b53d90
      Eric Myhre authored
      In the assembly, this ends up inlined, so there's no reduction in size,
      but there's also no reduction in speed.
      24b53d90
  10. 02 Feb, 2020 1 commit
    • Eric Myhre's avatar
      Finish recursive untyped map building. Tests. · 50275d24
      Eric Myhre authored
      Can you believe this required another wrapper type?  But indeed:
      this is the case: when building a recursive value, it is necessary for
      us to do some actions after that recursed operation becomes 'done'.
      In this case, we have to both kick the parent state machine along, and
      also finish the actual assignment into the map!  (In codegen maps,
      the latter need isn't true, for fun pointer dance reasons, but the
      need for state machine kicking -- as well as having the error checking
      branch to make sure any validation succeeded! -- is still applicable.)
      
      Tests all pass.
      50275d24
  11. 01 Feb, 2020 2 commits
    • Eric Myhre's avatar
      More of String style and builders; a hackme doc; more interface constraints. · 2f04140c
      Eric Myhre authored
      Starting to use a short little snippet of interface constraints at the
      top of every file is starting to feel useful again, both as a compiler
      sanity check and as a short readable inventory of the file.
      
      The hackme accumulates and clarifies a bunch of notes I found myself
      needing to make in one place as I try to settle the high level rules.
      It's mostly for the "ipldfree" implementations, and will move to that
      package when we're done with our research branch here and start moving
      things into their final places.  A few things might be more general;
      I'll sort those out into some separate file later.
      
      And plainString now has the full complement of NodeStyle, NodeBuilder,
      and all the other interfaces.
      
      In parallel but not committed here, I'm working on the 'any' node
      implementation for this new generation of "ipldfree".  That's turning
      out a bit more special (isn't everything in this project?) than
      expected: the ideal operation for NodeBuilder and NodeAssembler
      actually diverge, because the builder would like to return rather
      more less and more granular memory, whereas the assembler has no
      freedom to do so; this is the first occurance of such a divergence.
      So that's fun!  (I wonder if something like this will also come up
      for union types, later on.)
      2f04140c
    • Eric Myhre's avatar
  12. 22 Jan, 2020 1 commit
    • Eric Myhre's avatar
      More new map implementation. · 631a8e30
      Eric Myhre authored
      Includes duplicate key checks that were previously missing.
      
      Overall, checks many more invariants.  There are now "*_ValueAssembler"
      types involved in both the 'free'/generic implementation, and the
      codegen implementation: in both cases, it's needed for several tasks,
      mostly revolving around the "Done" method (or anything else that causes
      doneness, e.g. any of the scalar "Assign*" methods):
      
      - to make sure the map assembly doesn't move on until the value
        assembly is finished!  Need to do this to make it possible to
        maintain any other invariant over the whole tree!
      - to do state machine keeping on the map assembler
      - to do any integrity checks that the map itself demands
      - and in some cases, to actually commit the entry itself (although
        in some cases, pointer funtimes at key finish time are enough).
      
      The introduction of these '*_KeyAssembler' and '*_ValueAssembler' types
      is somewhat frustrating, because they add more memory, more vtable
      interactions (sometimes; in codegen, the compiler can inline them out),
      and just plain more SLOC.  Particularly irritatingly, they require a
      pointer back to their parent assembler... even though in practice,
      they're always embedded *in* that same structure, so it's a predictable
      offset from their own pointer.  But I couldn't easily seem to see a
      way around that (shy of using unsafe or other extreme nastiness), so,
      I'm just bitting the bullet and moving on with that.
      
      (I even briefly experimented with using type aliases to be able to
      decorate additional methods contextually onto the same struct memory,
      hoping that I'd be able to choose which type's set of methods I apply.
      (I know this is possible internally -- if one writes assembler, that's
      *what the calls are like*: you grab the function definition from a
      table of functions per type, and then you apply it to some memory!)
      This would make it possible to put all the child assembler functions
      on the same memory as the parent assembler that embeds them, and thus
      save us the cyclic pointers!  But alas, no.  Attempting to do this will
      run aground on "duplicate method" errors quite quickly.  Aliases were
      not meant to do this.)
      
      There's some new tests, in addition to benchmarks.
      
      'plainMap', destined to be part of the next version of the 'ipldfree'
      package, is now complete, and passes tests.
      
      A couple key tests are commented out, because they require a bump in
      version of the go-wish library, and I'm going to sort that in a
      separate commit.  They do pass, though, otherwise.
      
      Some NodeStyle implementations are introduced, and now those are the
      way to get builders for those nodes, and all the tests and benchmarks
      use them.  The placeholder 'NewBuilder*' methods are gone.
      
      There are some open questions about what naming pattern to use for
      exporting symbols for NodeStyles.  Some comments regard this, but it's
      a topic to engage in more earnest later.
      
      Benchmarks have been renamed for slightly more consistency and an eye
      towards additional benchmarks we're likely to add shortly.
      
      Some new documentation file are added!  These are a bit ramshackle,
      because they're written as an issue of note occurs to me, but there are
      enough high-level rules that should be held the same across various
      implementations of Node and NodeAssembler that writing them in a doc
      outside the code began to seem wise.
      631a8e30
  13. 13 Jan, 2020 1 commit
    • Eric Myhre's avatar
      Here's some stuff that can benchmark! · c51fb607
      Eric Myhre authored
      (Thank goodness.  Been in theoryland for a while.)
      
      There's somewhat more content here than necessary for the benchmark
      that's presently runnable; right now only the Map_K_T implementation
      is targetted.  I want benchmarks of things with complex keys in codegen
      and also benchmarks of the runtime/generic/free impls soon, so
      they can all be compared.
      
      There's also a quick fliff of stdlib map usage in a wee benchmark to
      give us some baselines...
      
      And there's also a quick fliff of stdlib json unmarshalling for the
      same reason.  It's not fair, to be sure: the json thing is doing work
      parsing, and allocating strings (whereas the other two get to use
      compile-time const strings)... but it sure would be embarassing if we
      *failed* to beat that speed, right?  So it's there to keep it in mind.
      
      Some off-the-cuff results:
      
      ```
      BenchmarkBaselineNativeMapAssignSimpleKeys-8             6815284               175 ns/op             256 B/op          2 allocs/op
      BenchmarkBaselineJsonUnmarshalMapSimpleKeys-8             724059              1644 ns/op             608 B/op         14 allocs/op
      BenchmarkFeedGennedMapSimpleKeys-8                       2932563               410 ns/op             176 B/op          8 allocs/op
      ```
      
      This pretty good.  If we're *only* half the speed of the native maps...
      that's actually reallyreally good, considering we're doing more work
      to keep things ordered, to say nothing of all the other interface
      support efforts we have to do.
      
      But 8 allocs?  No.  That was not the goal.  This should be better.
      
      Time to dig...
      c51fb607