Skip to content

Conversation

@oschwald
Copy link
Owner

Closes #36 and #41.

This PR addes a LookupNetwork method. This behaves similarly to Lookup except that it returns the network associated with the record in the database and an ok boolean indicating whether a record was found or not.

This also includes some minor cleanup and adds golangci-lint to Travis.

Benchmarks from the branch:

$ go test -bench=. -benchmem -benchtime=20s goos: linux goarch: amd64 pkg: github.com/oschwald/maxminddb-golang BenchmarkLookup-8 2000000 16109 ns/op 5360 B/op 205 allocs/op BenchmarkLookupNetwork-8 2000000 17076 ns/op 5417 B/op 208 allocs/op BenchmarkCountryCode-8	20000000 1222 ns/op 1 B/op 0 allocs/op PASS ok	github.com/oschwald/maxminddb-golang	125.193s 

Benchmarks of master + the err handling change:

$ go test -bench=. -benchmem -benchtime=20s goos: linux goarch: amd64 pkg: github.com/oschwald/maxminddb-golang BenchmarkMaxMindDB-8 2000000 16281 ns/op 5361 B/op 205 allocs/op BenchmarkCountryCode-8	20000000 1221 ns/op 1 B/op 0 allocs/op PASS ok	github.com/oschwald/maxminddb-golang	75.826s 
Copy link
Collaborator

@horgh horgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

reader.go Outdated
// result value to Decode into.
func (r *Reader) Lookup(ipAddress net.IP, result interface{}) error {
// Lookup retrieves the database record for ip and stores it in the value
// pointed to be result. If result is nil or not a pointer, an error is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"by result" I think. LookupNetwork()'s comment below as well.

reader.go Outdated
func (r *Reader) cidr(ip net.IP, prefixLength int) *net.IPNet {
// This is necessary as the node that the IPv4 start is at may
// be at a bit depth that is less that 96, i.e., ipv4Start points
// to a leaf node.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get this. It'd be in a database with no IPv4 IPs?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of. It could "have" them in the sense that lookups for them would return a result, but it would be the same result for all of them. For instance, if there was a record at ::/8.

pointer, prefixLength, ip, err := r.lookupPointer(ip)

network = r.cidr(ip, prefixLength)
if pointer == 0 || err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is 0 or an error, do we need to call cidr()?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that it would be useful to know the network size for empty networks as well as networks with records. I believe the legacy reader did return this. For errors, it might be less useful, although it could help in debugging a bad database.

Previously, we were benchmarking the assert.NoError call, which isn't particularly cheap.
when calculating LookupNetwork. Arguably, it might be cleaner to not set ipv4Start when there is no IPv4 subtree, although that would also introduce additional complications and it would make those lookups slower.
@oschwald oschwald force-pushed the greg/prefix-length branch from 23fc4df to 27395cf Compare August 27, 2019 17:47
This replaces the runtime check on record size and unrolls the node calculation. Before: ``` goos: linux goarch: amd64 pkg: github.com/oschwald/maxminddb-golang BenchmarkLookup-8 1000000 15928 ns/op BenchmarkLookupNetwork-8 1000000 16638 ns/op BenchmarkCountryCode-8	10000000 1244 ns/op PASS ok	github.com/oschwald/maxminddb-golang	46.654s ``` After: ``` goos: linux goarch: amd64 pkg: github.com/oschwald/maxminddb-golang BenchmarkLookup-8 1000000 14918 ns/op BenchmarkLookupNetwork-8 1000000 15681 ns/op BenchmarkCountryCode-8	20000000 1137 ns/op PASS ok	github.com/oschwald/maxminddb-golang	54.967s ```
Copy link
Collaborator

@horgh horgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@horgh horgh merged commit 78d62e5 into master Aug 28, 2019
@horgh horgh deleted the greg/prefix-length branch August 28, 2019 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants