Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

refactor: use a helper type to decode AddrInfo from JSON #178

Merged
merged 2 commits into from Feb 16, 2021

Conversation

Stebalien
Copy link
Member

This saves us a bunch of error prone manual decode/encode logic.

This saves us a bunch of error prone manual decode/encode logic.
@marten-seemann
Copy link
Contributor

This works, my fuzzer (not comitted yet) doesn't find any crashes. It requires extra allocations though, not sure if we should care about that.

@Stebalien
Copy link
Member Author

It requires extra allocations though, not sure if we should care about that

Really? I'd expect fewer allocations as we can avoid a lot of interface indirection.

Regardless, I don't think it will matter much either way.

@marten-seemann
Copy link
Contributor

This PR:

goos: darwin
goarch: amd64
pkg: github.com/libp2p/go-libp2p-core/peer
BenchmarkAddrInfoJSON-16    	  151012	      7853 ns/op	    2787 B/op	      46 allocs/op

#177:

goos: darwin
goarch: amd64
pkg: github.com/libp2p/go-libp2p-core/peer
BenchmarkAddrInfoJSON-16    	  135700	      8511 ns/op	    3658 B/op	      65 allocs/op

benchmark code:

func BenchmarkAddrInfoJSON(b *testing.B) {
	for i := 0; i < b.N; i++ {
		ai := AddrInfo{ID: testID, Addrs: []ma.Multiaddr{maddrFull}}
		out, err := ai.MarshalJSON()
		if err != nil {
			panic(err)
		}
		var addrInfo AddrInfo
		if err := addrInfo.UnmarshalJSON(out); err != nil {
			panic(err)
		}
		if addrInfo.ID != testID {
			panic(fmt.Sprintf("expected ID to equal %s, got %s", testID.Pretty(), addrInfo.ID.Pretty()))
		}
		if len(addrInfo.Addrs) != 1 || !addrInfo.Addrs[0].Equal(maddrFull) {
			panic(fmt.Sprintf("expected addrs to match %v, got %v", maddrFull, addrInfo.Addrs))
		}
	}
}

@marten-seemann
Copy link
Contributor

That's unambiguous. Let's use the PR. @Stebalien can you cherry-pick the test case I wrote for #177?

@Stebalien Stebalien merged commit adcd724 into master Feb 16, 2021
@Stebalien Stebalien deleted the refactor/simple-decode branch February 16, 2021 20:06
@Stebalien Stebalien mentioned this pull request May 11, 2021
27 tasks
@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants