Refactor for better caching #30

Stebalien · 2020-08-25T01:00:33Z

Motivation: Updates to the AMT usually involves multiple unnecessary writes during the modifications. This refactor punts all writes to the end.

This refactor:

Ensures we never write anything till we flush (and that we only write modified nodes). This:
1. Saves us quite a bit of time/gas when making many state modifications.
2. Gives us some room to further optimize batch operations without requiring a network upgrade.
3. Is much easier to reliably re-implement (e.g., in other languages).
Completely decodes nodes on load, and re-encodes them on save. This means all bitfield operations are isolated to two functions and bitfields do not need to be maintained in state (they're generated on the fly on flush).
Checks a bunch of invariants. We can't check everything, but we can at least avoid doing anything terribly incorrect.

Note: technically, we'll still:

Write nodes if we change them, then change them back. This is mostly unavoidable and not worth it.
Always write the root node. This is avoidable, but likely not worth it either.

why don't you fix that

-- @whyrusleeping

Stebalien · 2020-08-25T05:49:46Z

node.go

+
+	// If we have _no_ links, we've collapsed everything.
+	if nd.links[0] == nil {
+		return 0, nil


note: this is a bug in the current AMT. If we insert a high key, then delete it, we don't round-trip back to the empty AMT. (we fail to reset the height).

Stebalien · 2020-08-25T16:24:34Z

Results:

Speeds up BenchmarkAMTInsertBulk by 10% (likely by significantly more with an on-disk datastore).
Cuts loads/stores in BenchmarkAMTInsertBulk by 45%.

This refactor: 1. Ensures we never write anything till we flush. This: 1. Saves us quite a bit of time/gas when making many state modifications. 2. Gives us some room to further optimize batch operations without requiring a network upgrade. 3. Is much easier to reliably re-implement (e.g., in other languages). 2. Completely decodes nodes on load, and re-encodes them on save. This means all bitfield operations are isolated to two functions and bitfields do not need to be maintained in state (they're generated on the fly on flush). 3. Checks a bunch of invariants. We can't check everything, but we can at least avoid doing anything terribly incorrect.

Most batch-delete operations can be safely implemented without changing observed behavior (blockstore access patterns) as long as the indices are pre-sorted.

This patch set introduces a breaking change to the format (fixes a bug).

Stebalien · 2020-10-26T21:50:36Z

@ZenGround0 I've rebased this PR on master. Please review when you get a chance.

rvagg · 2020-10-27T01:16:47Z

in terms of the algorithm (discounting caching) this doesn't change anything except for that minor collapse fix does it?

Stebalien · 2020-10-27T01:24:31Z

Yes. It should (modulo the bug fix) produce the same CIDs).

anorth · 2020-10-27T04:01:26Z

Thanks @Stebalien. Should we repackage to v3 before landing this and related changes?

Stebalien · 2020-10-27T15:01:40Z

The last commit in this PR repackages to v3. The tricky part will be consuming v3 and v2 at the same time.

ZenGround0

Not done yet but dropping off some comments before I finish going through this tomorrow.

ZenGround0 · 2020-10-28T22:29:34Z

internal/internal.go

+	MaxIndexBits = 63
+	WidthBits    = 3
+	Width        = 1 << WidthBits             // 8
+	BitfieldSize = 1                          // ((width - 1) >> 3) + 1


nit: if this line did the calculation explicitly it would match the line above and constrain constants at runtime to avoid errors

Suggested change

BitfieldSize = 1 // ((width - 1) >> 3) + 1

BitfieldSize = ((Width - 1) >> 3) + 1 // 1

Oh I see the assert in the init function below. Any reason to prefer that over constraining as suggested above?

Also if there is someway to document that the magic number 3 here is log of number of bits in a byte and unrelated to WidthBits it may help preempt some confusion.

When this PR is merged I'll rebase and clean up my big docs PR #23, make it lighter-touch and easier to merge as just docs (currently has a couple of minor code changes to help with making it reader-friendly). That should help with making a lot of these magic values clearer.

IIRC, we thought the math was too confusing and opaque so we just asserted it. I'm going to clean this all up in my PR to make the width configurable.

ZenGround0 · 2020-10-28T22:43:31Z

amt.go

-	expLinks []cid.Cid
-	expVals  []*cbg.Deferred
-	cache    []*Node
+	store cbor.IpldStore
 }

 func NewAMT(bs cbor.IpldStore) *Root {
 	return &Root{
 		store: bs,


super nit: not your naming but fyi using bs for a cbor IpldStore always makes me do a double take when I come back to this code because this thing is close to but not quite a BlockStore. Probably not worth changing.

I agree this is a bit funky. We can address it in a followup PR.

ZenGround0 · 2020-10-29T04:16:33Z

node.go

+		if !expectLeaf {
+			return nil, errLeafUnexpected
+		}
+		for x := byte(0); x < internal.Width; x++ {


nit: can we drop the cast to byte to help avoid confusion? Reading through the dense bit operations on line 42 I kept getting thrown by byte x not having bit ops performed on it. I'm missing how the cast helps since the loop bounds should keep all array indexes in bounds.

I believe this was a micro optimization but it almost certainly doesn't make a difference. I'll remove it.

ZenGround0 · 2020-10-29T20:33:24Z

node.go

+			continue
+		}
+		if ln.dirty {
+			subn, err := ln.cached.flush(ctx, bs, height-1)


I don't think there is any possible code path that can hit this, but if the dirty bit is somehow set without a cached node then this will panic. What are your thoughts on checking for this condition and returning an error in this case? Is panicing preferable?

It should be an invariant and I'd usually panic in cases like this. But given the blockchain context, it probably makes sense to be extra defensive.

Stebalien requested review from whyrusleeping and ZenGround0 August 25, 2020 05:43

Stebalien commented Aug 25, 2020

View reviewed changes

Stebalien force-pushed the steb/rewrite branch 2 times, most recently from f18774b to e6282fa Compare August 25, 2020 06:45

austinabell added a commit to austinabell/go-amt-ipld that referenced this pull request Sep 23, 2020

Expanding on test for bug found in filecoin-project#30

f982b58

Stebalien force-pushed the steb/rewrite branch from 8e17758 to 948f5cf Compare September 28, 2020 20:48

Stebalien mentioned this pull request Sep 29, 2020

collapse the height of empty AMTs #34

Closed

Stebalien added 7 commits October 26, 2020 14:48

Enable future batch-delete optimizations

db5ca53

Most batch-delete operations can be safely implemented without changing observed behavior (blockstore access patterns) as long as the indices are pre-sorted.

Increase test coverage

5fd0069

Mark nodes as clean on flush

615b741

Collapse when we completely delete

680f449

Report put/get metrics

e6bde11

update to v3

9c15d17

This patch set introduces a breaking change to the format (fixes a bug).

Stebalien force-pushed the steb/rewrite branch from 948f5cf to 9c15d17 Compare October 26, 2020 21:49

ZenGround0 reviewed Oct 29, 2020

View reviewed changes

ZenGround0 approved these changes Oct 29, 2020

View reviewed changes

address feedback

cb3d5f0

Stebalien force-pushed the steb/rewrite branch from 04f4207 to cb3d5f0 Compare October 29, 2020 21:36

Stebalien merged commit b0c6e52 into master Oct 29, 2020

Stebalien deleted the steb/rewrite branch October 29, 2020 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor for better caching #30

Refactor for better caching #30

Stebalien commented Aug 25, 2020

Stebalien Aug 25, 2020

Stebalien commented Aug 25, 2020

Stebalien commented Oct 26, 2020

rvagg commented Oct 27, 2020

Stebalien commented Oct 27, 2020

anorth commented Oct 27, 2020

Stebalien commented Oct 27, 2020

ZenGround0 left a comment

ZenGround0 Oct 28, 2020

ZenGround0 Oct 28, 2020

ZenGround0 Oct 29, 2020

rvagg Oct 29, 2020

Stebalien Oct 29, 2020

ZenGround0 Oct 28, 2020

Stebalien Oct 29, 2020

ZenGround0 Oct 29, 2020

Stebalien Oct 29, 2020

ZenGround0 Oct 29, 2020

Stebalien Oct 29, 2020

	BitfieldSize = 1 // ((width - 1) >> 3) + 1
	BitfieldSize = ((Width - 1) >> 3) + 1 // 1

Refactor for better caching #30

Refactor for better caching #30

Conversation

Stebalien commented Aug 25, 2020

Choose a reason for hiding this comment

Stebalien commented Aug 25, 2020

Stebalien commented Oct 26, 2020

rvagg commented Oct 27, 2020

Stebalien commented Oct 27, 2020

anorth commented Oct 27, 2020

Stebalien commented Oct 27, 2020

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment