improve allocations in map serialization by whyrusleeping · Pull Request #105 · whyrusleeping/cbor-gen

whyrusleeping · 2024-10-28T16:50:33Z

reuse buffers more and use optimized read paths when available.

Before:

why@sirius ~/c/cbor-gen (master)> go test ./testing -run XXX -bench=.
goos: linux
goarch: amd64
pkg: github.com/whyrusleeping/cbor-gen/testing
cpu: AMD Ryzen Threadripper 3970X 32-Core Processor
BenchmarkMarshaling-64         	 1856397	       712.1 ns/op	      48 B/op	       1 allocs/op
BenchmarkUnmarshaling-64       	  199536	      6274 ns/op	    1777 B/op	      21 allocs/op
BenchmarkLinkScan-64           	  729843	      1645 ns/op	     112 B/op	       1 allocs/op
BenchmarkDeferred-64           	  505626	      2223 ns/op	      40 B/op	       2 allocs/op
BenchmarkMapMarshaling-64      	 2326390	       497.8 ns/op	      50 B/op	       3 allocs/op
BenchmarkMapUnmarshaling-64    	  140995	      8113 ns/op	    2043 B/op	      36 allocs/op
PASS
ok  	github.com/whyrusleeping/cbor-gen/testing	10.424s

After:

why@sirius ~/c/cbor-gen (master)> go test ./testing -run XXX -bench=.
goos: linux
goarch: amd64
pkg: github.com/whyrusleeping/cbor-gen/testing
cpu: AMD Ryzen Threadripper 3970X 32-Core Processor
BenchmarkMarshaling-64         	 1755950	       722.5 ns/op	      48 B/op	       1 allocs/op
BenchmarkUnmarshaling-64       	  192193	      6699 ns/op	    1777 B/op	      21 allocs/op
BenchmarkLinkScan-64           	  672280	      1669 ns/op	     112 B/op	       1 allocs/op
BenchmarkDeferred-64           	  656348	      2182 ns/op	      40 B/op	       2 allocs/op
BenchmarkMapMarshaling-64      	 2236473	       536.9 ns/op	      50 B/op	       3 allocs/op
BenchmarkMapUnmarshaling-64    	  183109	      6313 ns/op	    1930 B/op	      20 allocs/op
PASS
ok  	github.com/whyrusleeping/cbor-gen/testing	8.893s

Stebalien · 2024-10-28T17:10:11Z

utils.go

+// ReadFullStringIntoBuf will read a string off the given stream, consuming the
+// entire cbor item if the string on the stream is longer than the buffer,
+// the string is discarded and 'false' is returned
+func ReadFullStringIntoBuf(cr *CborReader, buf []byte, maxLength uint64) (int, bool, error) {


nit: generally I prefer the Hash version of this:

Take a zero-length buffer with some capacity.

Return a buffer containing the output, appending to the input buffer if possible.

That way it's possible to just pass nil and let the function do the allocation for you. You also won't need to handle slicing, etc.

So the main reason i added this was to avoid any allocations, i can make it append if necessary but it feels more prone to being misused in that case

Ah, I see. You want to be able to skip fields that are too long entirely. I guess that makes sense, it just seems like a bit of a sharp edge.

yeah... i would make it private but then generated files can't call it

ill add an extra comment

utils.go

Stebalien · 2024-10-28T17:14:21Z

peeker.go

 	return b, nil
 }

+func (p *peeker) ReadByteBuf(buf []byte) (byte, error) {


If peeker.ReadByte is allocating buf, maybe just replace peeker.lastByte with a [1]byte and use that directly in io.ReadFull?

ah, thats a good idea

io.go

Stebalien · 2024-10-29T17:37:01Z

gen.go

+
+		if !ok {
+			// Field doesn't exist on this type, so ignore it
+			cbg.ScanForLinks(cr, func(cid.Cid){})


Do we not have a better way to ignore fields? Honestly, I'd special-case Deferred into an Ignored type that just drops everything.

Also, we need to handle errors.

yeah thats the way we've been doing it so far, probably makes sense to have a more dedicated thing for it

Eh, this is probably fine (assuming you check for errors). It ensures that any CIDs contained in such fields are valid.

Stebalien · 2024-10-29T18:21:14Z

io.go

+	if len(scratchBuf) < int(extra) {
+		return cid.Undef, fmt.Errorf("scratchBuf not large enough for cid")


Don't we usually allocate header-sized scratch bufs?

yeah, this is actually a different path entirely (not used in codebase yet). I wanted this for doing some manual/optimized reading of certain objects

Stebalien · 2024-10-29T18:21:43Z

io.go

 	cr.r = GetPeeker(r)
 }

+func (cr *CborReader) ReadCid(scratchBuf []byte) (cid.Cid, error) {


Were you planning on using this somewhere?

outside of package, yeah. Its really a separate thing from the rest of this PR, happy to remove for now and bring back in later

It's just kind of awkward given that:

Everything else uses the internal scratch buffer.

It will fail if the scratch buffer isn't large enough.

If we need a CID scratch buffer, I'd consider just increasing the size of the internal scratch buffer to fit CIDs.

alright, ill just remove this for now. can bring it back later

Stebalien · 2024-10-29T18:28:24Z

gen.go

+
+		if !ok {
+			// Field doesn't exist on this type, so ignore it
+			cbg.ScanForLinks(cr, func(cid.Cid){})


Eh, this is probably fine (assuming you check for errors). It ensures that any CIDs contained in such fields are valid.

improve allocations in map serialization

b6b4c28

whyrusleeping requested review from Stebalien and magik6k October 28, 2024 16:50

Stebalien reviewed Oct 28, 2024

View reviewed changes

Stebalien reviewed Oct 29, 2024

View reviewed changes

whyrusleeping added 2 commits October 29, 2024 11:11

review cleanup

1256f96

extra comment for clarity

3299272

Stebalien reviewed Oct 29, 2024

View reviewed changes

more review feedback

bf19270

Stebalien approved these changes Oct 30, 2024

View reviewed changes

whyrusleeping merged commit b7a6831 into master Oct 30, 2024

BrewTestBot mentioned this pull request Aug 27, 2025

ipfs 0.37.0 Homebrew/homebrew-core#235214

Merged

		if len(scratchBuf) < int(extra) {
		return cid.Undef, fmt.Errorf("scratchBuf not large enough for cid")

Conversation

whyrusleeping commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants