- 01 Apr, 2020 3 commits
-
-
Eric Myhre authored
This cleans up a lot of stuff and reduces the amount of boilerplate content that's just generating monomorphizing error method stubs. The nodeGenerator mixins now also use the node mixins. I don't know why I failed to do that from the start; I certainly meant to. It results in shorter generated code, and the compiler turns it into identical assembly.
-
Eric Myhre authored
(That line kind of says a lot about why this project is tricky, doesn't it.)
-
Eric Myhre authored
Compiles and runs; but the results don't. Some small issues; some big issues. Just needed a checkpoint. It's feeling like it's time to deal with breaking down the assembler generators in the same way we did for all the node components, which is going to be kind of a big shakeup.
-
- 29 Mar, 2020 4 commits
-
-
Eric Myhre authored
I *think* the notes on Maybes are now oscillating increasingly closely around a consistent centroid. But getting here has been a one heck of a noodle-scratcher.
-
Eric Myhre authored
-
Eric Myhre authored
-
Eric Myhre authored
In research: I found that there are more low-cost ways to switch which methods are available to call on a value than I thought. Also, these techniques work even for methods of the same name. This is going to improve some code in NodeAssemblers significantly -- there are several situations where this will let us reuse existing pieces of memory instead of needing to allocate new ones; even the basicnode package now already needs updates to improve this. It's also going to make returning representation nodes from our typed nodes *significantly* easier and and lower in performance costs. (Before finding methodsets are in fact so feasible, I was afraid this was going to require our typed nodes to embed yet another small struct with a pointer back to themselves so we can have amortized availability of value that contains the representation's logic for the Node interface... which while it certainly would've worked, would've definitely made me sigh deeply.) Quite exciting for several reasons; only wish I'd noticed this earlier. Also in research: I found a novel way to make it (I believe) impossible to create zero values of a type, whilst also making a symbol available for it in other packages, so that we can do type assertions, etc, with that symbol. This is neat. We're gonna use this to make sure that types in your schema package can never be created without passing through any validation logic that the user applies. In codegen: lots of files disappear. I'm doing a tabula rasa workflow. (A bunch of the old files stick around in my working directory, and are being... "inspirational"... but everything is getting whitelisted before any of it ports over to the new commits. This is an effective way to force myself to do things like naming consistency re-checks across the board. And there's *very* little that's getting zero change since the changes to pointer strategy and assembler interface are so sweeping, so... there's very little reason *not* to tabula rasa.) Strings are reimplemented already. *With* representations. Most of the codegen interfaces stay roughly the same so far. I've exported more things this time around. Lots of "mixins" based on lessons learned in the prior joust. (Also a bunch of those kind-based rejections look *much* nicer now, since we also made those standard across the other node packages.) Some parts of the symbol munging still up in the air a bit. I think I'm going to go for getting all the infrastructure in place for allowing symbol-rename adjunct configuration this time. (I doubt I'll wire it all the way up to real usable configuration yet, but it'll be nice to get as many of the interventions as possible into topologically the right places to minimize future effort required.) There's a HACKME_wip.md file which contains some other notes on priorities/goals/lessoned-learned-now-being-applied in this rewrite which may contain some information about what's changing at a higher level than trying to track the diffs. (But, caveat: I'm not really writing it for an audience; more my own tracking. So, it comes with no guarantee it will make sense or be useful.)
-
- 26 Mar, 2020 1 commit
-
-
Eric Myhre authored
-
- 02 Mar, 2020 1 commit
-
-
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.
-
- 27 Feb, 2020 2 commits
-
-
Eric Myhre authored
This is the last part of the cleanup and cyclic import fix started in the previous commit.
-
Eric Myhre authored
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.)
-
- 30 Jan, 2020 1 commit
-
-
hannahhoward authored
rename EmitTypedLinkNodeMethodReferencedLinkBuilder to EmitTypedLinkNodeMethodReferencedNodeBuilder
-
- 29 Jan, 2020 1 commit
-
-
hannahhoward authored
-
- 07 Dec, 2019 4 commits
-
-
Eric Myhre authored
-
Eric Myhre authored
And expand on the need for both general and specific methods, even though they both can accomplish the same goals.
-
Eric Myhre authored
-
Eric Myhre authored
I'm still scratching out prototypes in another directory to make it easier to focus on the details I'm interested in rather than get wrapped up with the kerfuffling details of the existing full interfaces... as well as easier to try out different interfaces. And at some point, of course, we want *codegen* for these things, while what I'm plunking about with here is a sketch of the expected *output*. So, this is a large step removed from the final code... but it's easier to sketch this way and "imagine" where the templating can later appear. This solution approach seems to be going largely well. And we have some tests which count allocs very carefully to confirm the desired result! Still a lot to go. There's large hunks of comments and unresolved details still in this commit; I just need a checkpoint. Some things around creation of maps are still looking tricky to support optimizations for. Research needed there. A comment hunk describes the questions, but so far there's no tradeoff-free answer. Perhaps most usefully: here's also a checkpoint of the "HACKME_memorylayout" document! It also still has a few todos, but it's the most comprehensive block of prose I've got in one place so far. Hopefully it will be useful reading for anyone else wanting to get up to speed on the in-depth in "why" -- it's awfully, awfully hard to infer this kind of thing from the eschatology of just observing where pointers are in a finished product! Signed-off-by: Eric Myhre <hash@exultant.us>
-
- 08 Nov, 2019 1 commit
-
-
hannahhoward authored
Add a disclaimer to the header for generated code to prevent linters from complaining
-
- 07 Nov, 2019 1 commit
-
-
hannahhoward authored
Map/List should return an actual node type, not a pointer to a node. Otherwise, breaks when nested in other map/list type
-
- 25 Oct, 2019 6 commits
-
-
Eric Myhre authored
Using pointers for optional or nullable fields forces a lot of things to need allocations on the heap. And maybe that's not so good. So here's a very different take. 'Maybe' structures for any optional or nullable fields store the extra information about null or absent values. This takes more linear memory... but should tend to result in fewer allocs. It's also much easier to code against, I suspect. (And we expect it's not unreasonable for users to add more methods in the generated output package, which would mean we should be believable to code against.)
-
hannahhoward authored
Make emit methods public to allow a preliminary method for generating code outside this module
-
Eric Myhre authored
Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
Rearrange to match 68b3383d, and add methods to match 41d79374. Everything compiles again, and resultant code passes what tests it has. Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
(n.b. I've already tried implementing the suggested thing for structs, but won't link to it here as that commit is almost certainly going to end up rebased. Jury's still out on if the idea is good or not. Seems like the answer is almost certainly "it depends".)
-
Eric Myhre authored
-
- 24 Oct, 2019 2 commits
-
-
hannahhoward authored
fill in generated type for list
-
hannahhoward authored
Fill in generated types for int, link, and bytes nodes
-
- 17 Oct, 2019 2 commits
-
-
Eric Myhre authored
The previous names started to seem more questionable as we start generating additional code which is natively typed rather than interface constrained. Now the latter stuff is in a file that includes 'Node' as part of the filename. This seems better.
-
Eric Myhre authored
This introduces several new methods to the type generator. The new comment at the top of 'gen.go' explains the direction. There are several (sizable impact) TODOs in the methods for structs; this is because some other research I've been doing on performance is going to result in a re-think of how we regard pointers, and a *lot* of the struct code is going to get a shakeup shortly. Should be coming up in about two commits or so. (The 'Maybe' structs getting their first mention here will have something to do with it!) Some more file split-ups to keep node interface generation separate from native-typed API generation will be coming up next commit. You can also see here some TODOs regarding the future possibility of "validate" methods. This is something I want to pursue, but the implementation work will be nontrivial -- those TODOs will probably stay there a good while. Signed-off-by: Eric Myhre <hash@exultant.us>
-
- 15 Oct, 2019 1 commit
-
-
Eric Myhre authored
-
- 14 Oct, 2019 1 commit
-
-
Eric Myhre authored
(At long last! These child-builder methods are a step that was known to be needed for some time now, but I've intentionally been pushing off the implementation of it until we finally reached some *other* piece of functionality that would depend on it and force the issue. Finally, that's happened: that issue is unmarshalling into natively-typed codegenerated nodes, and that's now. Woo!) (Though the current focus of this is for natively-typed codegen'd nodes, if we put further effort into reflective-bind node implementations, they'll also lean heavily on these child-builder-getters for internally tracking the correct `reflect.Type` to create new values of -- very similar to what our schema-typed things are doing, just with the Golang type system instead of our schema types.) The child-builder getter methods more or less explain themselves. They yield a new NodeBuilder that's correctly specialized for child nodes in recursive structures. This is necessary for both keys and values in maps, and for values in lists. For values in lists and maps, getting the child-builder requires takes a parameter -- this is so typed nodes (namely, structs; but some unions will also lean on this) may have discriminated contents that differ per key or index. The BuilderForValue functions are able to take primitive strings and ints as parameters rather than Node, because all uses that have discriminated results are places where complex keys are not acceptable. Therefore primitives are fine, and preferred since it avoids boxing. The 'free' node implementation is updated to include this feature, as are codegenerated nodes. (The runtime wrapper typed node implementations have partial support, but stubbed methods where the path forward is clear but the implementation effort nontrivial and not currently on-priority-path.) Unmarshalling now uses these child-builder-getters to correctly handle the passing down of constraints and typeinfo. This addresses several TODO's that have been in the unmarshalling code for aeons -- hooray! (Somewhat remarkably, our solution here is much better than the one proposed in those ancient TODO's -- we actually got this done *without* direct abstraction-breaking examination for typed nodes. Nice!) Unmarshalling tests are also in this same diff. There's even a wee little benchmark using a snippet of JSON as fixture. This is the first time we see unmarshalling and marshalling end-to-end connected with codegen'd nodes. This is very exciting! There's still some questions to be answered about the most correct (and most performance-friendly) place to put error handling against asks for a child NodeBuilder with an invalid key or index, which is a very real invalid state that's reachable for typed nodes that are structs. Comments about that are inline in the diff. We'll probably have to come back to that soon, but I'm gonna let it stew on the back burner for a bit. Currently, panics are used, but this may not be acceptable ergonomics. --- It's somewhat effortful to even detail how many thoughts about performance and the future optimization potentials go into this diff. Some comments I had in the area of the child-builder specialization functions included concerns on: - where common-subexpression-elimination will and won't apply - where inlining is possible vs facing function call stack shuffle overhead - ... as well as how that affects common-subexpression-elimination - ... as well as how that affects escape analysis and shifts to heap - whether an isDiscriminated property will make any of these things better - can BuilderForValue hang onto a builder, boxed into interface already, when appropriate? - ... for the maps case, should amortize in a way comparable to isDiscriminated, yes? verify. - ... can we do that without the builder keeping another two words of memory just... around? i don't think so. question is if it pays for itself. - question: do zero value no-fields structs get a bonus? reading runtime/iface.go -- doesn't look like it. but needs confirmation. - ... revised statement: they do, it's just nonobvious. iface.go doesn't have a specialization, but the mallocgc call hits a special path internally for zero, and then the typedmemmove call 'coincidentally' hits its "src == dst" short-circuit, thus also being fast. ... all in all, as many of these as possible of these concerns have been considered and this design should be optimization-friendly; some of them are bucketed into "we'll see", because we need to build nontrivial programs to see how significant the needs are, as well as to see how clever the compiler can get with what we've already done. There are some possible alternative ways we might re-engineer the builders in the future to make them more performance-friendly. In particular, the fact that many builders carry a previous value with them in anticipation of one of the 'Amend' calls -- which is not necessarily going to be used! -- is a potential obstruction to optimization, since it makes many structures go from zero to more-than-zero size, and that hits significantly different paths in the runtime with very different performance characteristics. However, that is a bit much to change today; so the thought lives here as a note for potential future work. Adding a `BuilderForValueIsDiscriminated() bool` method later seems still seems on the table, but since it wouldn't require substantial restructurings or headaches, can be deferred until benchmarks show us exactly how much it matters. (A single `type ChildBuilders interface { Style() Complexity; ... }` with some enum for `Compleixty` resembling `SimpleList | SimpleMap | DiscriminatedMap | DiscriminatedList` might also be a tempting way to go about this.) --- An alternative design considered that deserves a quick note: we could've added a `ChildBuilders()` method to the MapBuilder and ListBuilder, and attached the further getter methods there. This might still be viable, if we found that there are more methods involved in the future and we want to group them: it *should* always land on the zero-fields struct path, which means it can be done without hitting an expensive malloc. However, at present this doesn't seem warranted: we don't have enough methods to make it feel important (especially after the recognition that Node variants of methods aren't needed in addition to the primitive args variants, described above). --- Whew. Lines of code in diff does not correlate to hours of thought required. Glad to have this landing. Even performance minutae aside, there was a significant period of time where I was terrified that all these abstractions were going to shatter magnificently when it came time to do the final end-to-end unification of all this massive arc of work. But now it's done. And it works. Whew. Next: more fleshing out of more types; perhaps more benchmarks; and also it's time to do some more drafts of the native accessors and builders. Some of that already has been done in parallel in gists while this diff was incubating, but it's time to commit it too.
-
- 03 Sep, 2019 1 commit
-
-
Eric Myhre authored
All flies right, yey We also get to shorten up the asserts drastically in this set, because we can just flat out assert that the result equals exactly the value that we got from the type-level builder earlier. (And go-cmp output from go-wish makes this very, very easy.) Signed-off-by: Eric Myhre <hash@exultant.us>
-
- 01 Sep, 2019 4 commits
-
-
Eric Myhre authored
I'm not sure if I like these symbol names. Or rather, I don't, but haven't decided on what would be preferable yet. There's a couple of options that have come to mind: - option 1 - `func {{ .Type.Name }}__NodeBuilder() ipld.NodeBuilder` - `func {{ .Type.Name }}__ReprBuilder() ipld.NodeBuilder` - option 2 - `func NewBuilderFor{{ .Type.Name }}() ipld.NodeBuilder` - `func NewReprBuilderFor{{ .Type.Name }}() ipld.NodeBuilder` - option 3 - `func (Builders) {{ .Type.Name }}() ipld.NodeBuilder` - `func (ReprBuilders) {{ .Type.Name }}() ipld.NodeBuilder` Option 3 would make 'Builders' and 'ReprBuilders' effectively reserved as type names if you're using codegen. Schemas using them could use adjunct config specific to golang to rename things out of conflict in the generated code, but it's still a potential friction. Option 2 would also have some naming collision hijinx to worry about, on further though. Only Option 1 is immune, by virtue of using "__" in combination with the schema rule that type names can't contain "__". This diff is implementing Option 1. I think I'm partial to Option 3, but not quite confident enough in it to lunge for it yet. Putting more methods on the *concrete* types would also be another interesting fourth option! These methods would ignore the actual value, and typically be used on the zero value: e.g., usage would resemble `Foo{}.ReprBuilder()`. The upside of this would be we'd then have no package scoped exported symbols except exactly the set matching type names in the schema. However, the opportunities for confusion with this would be numerous: we couldn't use the 'NodeBuilder' method name (because that's the potentially-stateful/COW one), but would still be returning a NodeBuilder type? Etc. Might not be good. More to think about here in the future. Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
Fixed at least one bug along the way (in iterators, which don't have test coverage yet, so no test fix. Still planning to cover those via serialization, when we get that feature, "soon"). 'go doc .' on the generated code now only lists one type per type in the schema which seems like a good sanity heuristic; and 'go doc -u .' on the package now looks much more consistent. (There's *8* types for every struct in the schema! Uffdah. But if that's what it takes to make a focused, correctness-emphasizing library surface area, so be it.) Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
I'm still aiming to keep this as simple and un-clever as possible, because putting lipstick on a pig -- this is all about to become strings which get shoveled back to a compiler parser anyway -- is not useful, and becomes antiuseful if it obstructs readability... But I'm starting to find these elements are repeated enough that it will help rather than hurt readability to extract some things. Also, since the munges have recently started to appear in both go code source as well as in the templates, that starts to put more weight in favor of extracting a function for it, which keeps the two syntactic universes from drifting on this subject. At the same time, moved all NodeBuilders to being unexported (by using idents prefixed with a "_"). I looked at the godoc for the generated code and felt this is looking like a wiser choice than exporting. We'll need to export more methods for getting initial instances of the now-unexported stuff... but we should be adding those anyway, so this is not an argument against unexporting. Some additional type idents around iterators and map builders have not yet been hoisted to DRYed munge methods. I'm debating if that's useful (as you can see in the comments in that file), but leaning towards it being more parsimoneous to just go for it. So that'll probably be the next commit. Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
And fixed tests for same. The value returned is still ipld.Undef, and I'm not sure I'm going to particularly defend that choice vs returning null, but it seems six of one and half a dozen of the other. Might be worth review later, once we have some field reports on whether that vs nils would be recieved as the more surprising/annoying choice in non-toy usage.
-
- 30 Aug, 2019 1 commit
-
-
Eric Myhre authored
Fix traversal internals to use it (!) rather than converting segments to strings, which was both wasteful, and in some cases, *wrong* (!) (although by coincidence happened to mostly work at present because of another thing from early-days code that was also technically wrong). Fix ipldfree.Node to reject LookupString if used on a list node! (This is the other "wrong" thing that made the traversal coincidentally work.) LookupSegment method generation also added to codegen. Signed-off-by: Eric Myhre <hash@exultant.us>
-
- 26 Aug, 2019 3 commits
-
-
Eric Myhre authored
It does vary from the baselines somewhat. There's a fixme in the codegen for struct-with-repr-map for where to address the bug found in the test commit preceeding this one, but solving it well depends on another change I've been planning in core, so I'm probably going to park this branch soon and go do that work on master; then come back to fix this here after that lands. Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
Mostly passes, but found one semantic bug. Signed-off-by: Eric Myhre <hash@exultant.us>
-
Eric Myhre authored
Just getting all the indentation changes out of the way. New tests coming in the next commit.
-