*: migrate to netaddr #540

jzelinskie · 2021-07-05T22:32:40Z

I'm sorry for the huge commit. This ended up touching far more than I thought it would.
I'm interested in seeing some perf numbers on real workloads before merging.

This change impacted almost the entire codebase:

All uses of net.IP are now netaddr.IP or netaddr.IPPort
Scrape now passes an entire peer
Testing in bittorrent package refactored
Parsing and API changes to Peers, PeerIDs, ClientIDs
Redis Storage now uses shorter keys

mrd0ll4r

Some performance and memory things I disagree with, and a lot of small things :)

frontend/udp/parser.go

storage/memory/peer_store.go

bittorrent/bittorrent.go

mrd0ll4r · 2021-07-06T13:11:25Z

storage/redis/peer_store.go

+// "I4" => number of IPv4 InfoHashes
+// "I6" => number of IPv6 InfoHashes
+func infohashCountKey(ip netaddr.IP) string {
+	var b strings.Builder


I wonder if the Builder is necessary or actually adds overhead. We know this is gonna be two bytes, so we might as well just allocate a slice for that.

storage/redis/peer_store.go

mrd0ll4r

I just saw the one buffer growth thing. Apart from that: It's a big one, not sure if I saw everything... maybe we can/should run the various benchmarking things? tbomb for just raw performance, and the storage memory usage thing (although I haven't tried that out in ages and don't know if it still works).

mrd0ll4r · 2021-07-07T09:20:43Z

storage/redis/peer_store.go

+	var b strings.Builder
+	switch {
+	case ip.Is4(), ip.Is4in6():
+		b.Grow(1 + 1 + 4) // "4" + "S"/"L" + IPv4


Missing 20 bytes for the actual infohash 🙃

nice catch

I also noticed that in PutLeecher it does not check if the infohash was new. It's apparently missing a

if reply[1] == 1 { // The Infohash Key (and thus the infohash) was new. _, err = conn.Do("INCR", infohashCountKey(ip)) if err != nil { return err } }

Is that on purpose? Should I add this?

I believe that's on purpose, because for redis we only count infohashes with at least one seeder. I don't remember why I did it like that, but I assume there was some reason for it...maybe it would have been difficult to correctly determine whether a seeder-only or leecher-only swarm already exists or something, not sure.

Aranjedeath · 2021-07-08T15:36:44Z

Yay!!! Thank you all. @mrd0ll4r we are running this branch in production currently :) I use regular memory storage backend, so redis issue does not affect me.

mrd0ll4r · 2021-07-08T16:37:18Z

Oh nice @Aranjedeath ! I can see the reboot and most things look fine so far. One thing I can't see is CPU usage (and request durations iirc), let us know how that works out :)

Aranjedeath · 2021-07-08T19:26:01Z

Oh nice @Aranjedeath ! I can see the reboot and most things look fine so far. One thing I can't see is CPU usage (and request durations iirc), let us know how that works out :)

IPV6 has request timing enabled, we disabled it on v4 previously for performance :)

Aranjedeath · 2021-07-12T00:41:06Z

Oh nice @Aranjedeath ! I can see the reboot and most things look fine so far. One thing I can't see is CPU usage (and request durations iirc), let us know how that works out :)

OK I think we have performance details in v4 now :)

This change impacted almost the entire codebase. - All uses of net.IP are now netaddr.IP or netaddr.IPPort - Scrape now passes an entire peer - Testing in bittorrent package refactored - Parsing and API changes to Peers, PeerIDs, ClientIDs - Redis Storage now uses shorter keys

jzelinskie · 2021-07-26T04:14:59Z

I pushed a commit that migrates us to zerolog in this branch. It's for convenience for testing. If you want me to open it as another PR after this, I can.

mrd0ll4r · 2021-08-23T09:23:45Z

bittorrent/bittorrent.go

@@ -14,6 +14,7 @@ import (
 	"inet.af/netaddr"

 	"github.com/chihaya/chihaya/pkg/log"
+	"github.com/jzelinskie/stringz"


mrd0ll4r

Every time I look at this I find some new small things..
I think it's good overall and I want it merged, but the PR is too big for me to confidently review it 🙃

Can we somehow split it up maybe?

mrd0ll4r · 2021-09-12T15:22:21Z

bittorrent/sanitize.go

-	log.Debug("sanitized scrape", r, log.Fields{
-		"maxScrapeInfoHashes": maxScrapeInfoHashes,
-	})
+	log.Debug().Uint32("maxScrapeInfoHashes", maxScrapeInfoHashes).Msg("sanitized scrape")


This is missing the sanitized request

mrd0ll4r · 2021-09-12T15:23:53Z

bittorrent/sanitize.go

+		r.IPPort = r.IPPort.WithIP(netaddr.IPFrom4(ip.As4()))
+	}
+
+	log.Debug().


This is also missing the request

mrd0ll4r · 2021-09-12T15:37:16Z

storage/storage_bench.go

@@ -435,7 +437,7 @@ func AnnounceSeeder1kInfohash(b *testing.B, ps PeerStore) {
 // ScrapeSwarm can run in parallel.
 func ScrapeSwarm(b *testing.B, ps PeerStore) {
 	runBenchmark(b, ps, true, putPeers, func(i int, ps PeerStore, bd *benchData) error {
-		ps.ScrapeSwarm(bd.infohashes[0], bittorrent.IPv4)
+		ps.ScrapeSwarm(bd.infohashes[0], bd.peers[0].IPPort.IP())


I think these should be pulled outside the runner function

mrd0ll4r · 2021-09-12T15:37:25Z

storage/storage_bench.go

@@ -445,7 +447,7 @@ func ScrapeSwarm(b *testing.B, ps PeerStore) {
 // ScrapeSwarm1kInfohash can run in parallel.
 func ScrapeSwarm1kInfohash(b *testing.B, ps PeerStore) {
 	runBenchmark(b, ps, true, putPeers, func(i int, ps PeerStore, bd *benchData) error {
-		ps.ScrapeSwarm(bd.infohashes[i%1000], bittorrent.IPv4)
+		ps.ScrapeSwarm(bd.infohashes[i%1000], bd.peers[0].IPPort.IP())


jzelinskie · 2021-11-09T20:34:58Z

I'll break this down and rework everything now that netaddr got added to the standard library as net/netip

jzelinskie requested a review from mrd0ll4r July 5, 2021 22:32

jzelinskie force-pushed the netaddr branch 7 times, most recently from f036b87 to c5b626f Compare July 5, 2021 23:42

mrd0ll4r requested changes Jul 6, 2021

View reviewed changes

jzelinskie force-pushed the netaddr branch 5 times, most recently from ae7f43f to df28e7e Compare July 7, 2021 05:14

mrd0ll4r requested changes Jul 7, 2021

View reviewed changes

jzelinskie added 4 commits July 20, 2021 14:42

.github: gofumpt/revive for fmt and lint

4c8d230

frontend/http: remove superfluous WriteHeader call

cb131f7

bittorrent: add PeerFromString()

a4ea67c

jzelinskie force-pushed the netaddr branch from df28e7e to a4ea67c Compare July 20, 2021 18:42

jzelinskie marked this pull request as ready for review July 20, 2021 18:42

jzelinskie added area/performance component/frontend/http component/storage labels Jul 20, 2021

jzelinskie force-pushed the netaddr branch 2 times, most recently from c8c3fd4 to 892fcea Compare July 26, 2021 04:13

pkg/log: migrate to zerolog

9f03e42

jzelinskie force-pushed the netaddr branch 2 times, most recently from 866886f to 0a75d53 Compare July 27, 2021 01:50

frontend/udp: replace sha256 with xxhash

5f86015

jzelinskie force-pushed the netaddr branch from 0a75d53 to 5f86015 Compare July 27, 2021 01:53

mrd0ll4r reviewed Aug 23, 2021

View reviewed changes

polarathene mentioned this pull request Aug 30, 2021

State of Chihaya? No release since 2016? Will a new release be published? #547

Open

mrd0ll4r requested changes Sep 12, 2021

View reviewed changes

jzelinskie mentioned this pull request Jan 2, 2022

Explore replacing net.IP with inet.af/netaddr.IP #538

Open

jzelinskie mentioned this pull request Apr 14, 2022

*: migrate to netip #567

Open

mrd0ll4r mentioned this pull request Feb 2, 2023

storage: modify interface to pass a slice on AnnouncePeers #602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: migrate to netaddr #540

*: migrate to netaddr #540

jzelinskie commented Jul 5, 2021

mrd0ll4r left a comment

mrd0ll4r Jul 6, 2021

mrd0ll4r left a comment

mrd0ll4r Jul 7, 2021

jzelinskie Jul 7, 2021

mrd0ll4r Jul 7, 2021

Aranjedeath commented Jul 8, 2021

mrd0ll4r commented Jul 8, 2021

Aranjedeath commented Jul 8, 2021

Aranjedeath commented Jul 12, 2021

jzelinskie commented Jul 26, 2021

mrd0ll4r Aug 23, 2021

mrd0ll4r left a comment

mrd0ll4r Sep 12, 2021

mrd0ll4r Sep 12, 2021

mrd0ll4r Sep 12, 2021

mrd0ll4r Sep 12, 2021

jzelinskie commented Nov 9, 2021

*: migrate to netaddr #540

Are you sure you want to change the base?

*: migrate to netaddr #540

Conversation

jzelinskie commented Jul 5, 2021

mrd0ll4r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrd0ll4r 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

Aranjedeath commented Jul 8, 2021

mrd0ll4r commented Jul 8, 2021

Aranjedeath commented Jul 8, 2021

Aranjedeath commented Jul 12, 2021

jzelinskie commented Jul 26, 2021

Choose a reason for hiding this comment

mrd0ll4r 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

jzelinskie commented Nov 9, 2021