trie: refactor stacktrie

[holiman]

This change refactors stacktrie to separate the stacktrie itself from the internal representation of nodes: a stacktrie is not a recursive structure of stacktries, rather, a framework for representing and operating upon a set of nodes.

I think it makes it easier to reason about, and also easier to deal with shared assets such as a hasher, the 'owner', and 'writeFunc' by having an envelope, rather than passing these things along at every constructor.

This way of doing it also opens up for some new optimizations: e.g. we could do without global pool for nodes, and use a local non-threadsafe pool which is only accessible internally. (Example: 8418248)

This PR is not yet complete, and it also does away with the binary serialization. Not sure if it's something we want to keep: unused code has a tendency to bitrot. The binary serialization has been added back, in it's own file. However, since the 'owner' of the trie now is moved to top level and not repeated in every node, the format was changed. Thus, I removed the original constructor so that this format-change must cause a compiler failure. But in general, I don't know if this really is something we want to kee, since we're not using it ourselves.

We don't have an awful lot of benchmarks for the stacktrie, afaict only one really, which indicates a slight improvement in this PR (as for why the mem went up, that's curious, I can probably get that down again).

name                       old time/op    new time/op    delta
DeriveSha200/std_trie-8       834µs ± 3%     848µs ± 1%    ~     (p=0.421 n=5+5)
DeriveSha200/stack_trie-8     514µs ± 5%     483µs ± 5%  -5.98%  (p=0.032 n=5+5)

name                       old alloc/op   new alloc/op   delta
DeriveSha200/std_trie-8       289kB ± 0%     290kB ± 0%  +0.09%  (p=0.008 n=5+5)
DeriveSha200/stack_trie-8    43.5kB ± 0%    46.3kB ± 0%  +6.48%  (p=0.008 n=5+5)

name                       old allocs/op  new allocs/op  delta
DeriveSha200/std_trie-8       2.91k ± 0%     2.92k ± 0%  +0.03%  (p=0.016 n=5+4)
DeriveSha200/stack_trie-8     1.25k ± 0%     1.26k ± 0%  +0.96%  (p=0.008 n=5+5)

[MariusVanDerWijden]

LGTM

[MariusVanDerWijden]

I spent a long time stuck on this.
Since we just set the children to nil, we could loose a lot of *stNodes that are not in the pool. However I finally came to the conclusion that this is not an issue since we only ever return *stNodes to the pool on

• hashRec of extNodes, which have no children themselves (extNode)
• hashRec of branchNodes, where we hashRec'd the children beforehand, thus returned the *stNodes already

So we will not loose allocations here (afaict)

[holiman]

Yeah, this is probably not even needed. I tested this:

		if st.children[i] != nil {
			panic(fmt.Sprintf("Child was %T", st.children[i]))
		}

But none of the tests triggered it. So the loop could probably be removed, but OTOH I think it's sane to have it there, to prevent potential mem leaks. It kind of makes sense that the pool-handling is robust in itself, without trusting too much that the "other parts" is correct.

[rjl493456442]

However, since the 'owner' of the trie now is moved to top level and not repeated in every node, the format was changed. Thus, I removed the original constructor so that this format-change must cause a compiler failure. But in general, I don't know if this really is something we want to kee, since we're not using it ourselves.

Actually owner doesn't need to be persisted in the binary format, which is just a "namespace" of this stackTrie, but not the core part of it. It's totally fine to remove owner from binary format.

[rjl493456442]

I would prefer to keep the owner and writeFn unchanged.

I guess the semantics of "Reset" is something we want to debate or discuss. Basically, there are two options:

Reset the referenced nodes but keep the settings for this trie (owner, writeFn, etc).
Reset the referenced nodes and set all the settings to nil.
The first option sounds better to me, though.

[holiman]

the owner and writeFn unchanged.

I think that is dangerous. The idea of Reset is that it can be called before entering it into the pool, where the object may live indefinitely. So we need to clear out anything which may hold memory references, and IMO the writeFn is very much a likely culprit to have references to local variables in local scopes.

The owner we might do away with completely, as you pointed out below.

[rjl493456442]

It's stackTrie, not stackNode.

[holiman]

Well, regardless: an object that is newly Reset should not hold external references, IMO

[rjl493456442]

I think it's a good refactor at the first glance, have a few nitpicks, will check the details tomorrow.

Btw, we should run stackTrie fuzzer a bit to ensure nothing is broken.

[rjl493456442]

lgtm, but please run fuzzer/stackTrie and maybe snap sync to ensure nothing is broken

[holiman]

Originally I think the binary marshalling was implemented so that we could pause/resume during sync. We didn't make use of that. @karalabe do you have any preference, whether to keep the feature or remove it?

I am a bit loathe to keep maintaining behaviour that we don't use (and have no reason to believe that anyone else is using either)

[rjl493456442]

Deployed it on 5/6, successfully finished the snap sync.