maxmind / mmdbwriter

Go library for writing MaxMind DB (mmdb) files
Apache License 2.0
116 stars 28 forks source link

Proposal: Represent Uint128 as two uint64 instead of big.Int #39

Open Fryuni opened 2 years ago

Fryuni commented 2 years ago

A big.Int value has 32 bytes in size

The number that it is holding only has 16 bytes, so using a big.Int adds an extra allocation, indirection and triples the memory use.

I propose the type Uint128 to be changed to:

type Uint128 struct { High, Low uint64 }

There are libraries in the community that define a Uint128 with its operations. I don't think there is a need for any operations here, so we could have the serialization and deserialization hand-coded here (using the binary package). If we don't want to deal with those operations we could depend on one such library.

For consumers that already have a big.Int we could provide a Uint128FromBig(*big.Int) Uint128 function.

This would be a braking change on the API, but that is ok by the status on the README.

oschwald commented 2 years ago

I am curious what your use case for the uint128 data type is. Currently, we don't actually use it and it hasn't received much thought. big.Int was chosen as that is what github.com/oschwald/maxminddb-golang](https://github.com/oschwald/maxminddb-golang) uses for deserialization. If this change is made, it is likely that additional changes will need to be made for loading an existing database containing the values.

When I wrote the reader, I was hoping that something would eventually happen with https://github.com/golang/go/issues/9455, but it has been almost 8 years without movement.

Fryuni commented 2 years ago

Currently just for including an IPv6 in the data (for custom aliasing). It is not a huge deal since it is only a few hundred of those, but using a pointer type larger than the data it points to felt very odd to me.