New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimizations #10

Merged
merged 5 commits into from Jun 13, 2017

Conversation

Projects
None yet
2 participants
@achille-roussel
Contributor

achille-roussel commented Jun 8, 2017

I noticed that the parsing and formatting operations of KSUIDs were quite inefficient regarding memory allocations, so I spent a bit of time optimizing them.

The biggest gain came from appending to the output buffer when converting from one base to another (instead of prepending) and reversing it at the end. I also changed some internal APIs to make it possible to pass preallocated buffers, and added the KSUID.Append([]byte) []byte function to allow formatting of KSUID with zero dynamic memory allocations.

Here are the numbers before the changes:

BenchmarkAppend-4   	  200000	      6539 ns/op	    1272 B/op	      95 allocs/op
PASS
ok  	github.com/segmentio/ksuid	1.385s

Then after:

BenchmarkAppend-4   	  300000	      4297 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/segmentio/ksuid	1.339s

And the comparison:

benchmark             old ns/op     new ns/op     delta
BenchmarkAppend-4     6539          4297          -34.29%

benchmark             old allocs     new allocs     delta
BenchmarkAppend-4     95             0              -100.00%

benchmark             old bytes     new bytes     delta
BenchmarkAppend-4     1272          0             -100.00%

Please take a look and let me know if anything should be changed.

bs := in[:]
func appendBase2Base(dst []byte, src []byte, inBase int, outBase int) []byte {
bs := src[:]
bq := [stringEncodedLength]byte{}

This comment has been minimized.

@rbranson

rbranson Jun 9, 2017

Contributor

So the original base2base function is generic: it works with arbitrary-length bytestrings. This change appears to maybe violate that in a way that is pretty sneaky? From what I'm reading it looks like bq is here to avoid allocating a []byte for each run of the for loop below and that it's pre-set to the size of the stringEncodedLength to avoid allocations from resizing the slice during appends. If a longer string comes in here is this just harmless (it's given the capacity of the size used in this library) or will it just panic and die?

@rbranson

rbranson Jun 9, 2017

Contributor

So the original base2base function is generic: it works with arbitrary-length bytestrings. This change appears to maybe violate that in a way that is pretty sneaky? From what I'm reading it looks like bq is here to avoid allocating a []byte for each run of the for loop below and that it's pre-set to the size of the stringEncodedLength to avoid allocations from resizing the slice during appends. If a longer string comes in here is this just harmless (it's given the capacity of the size used in this library) or will it just panic and die?

This comment has been minimized.

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

It doesn't change the original behavior, bq here is used as the initial backend array, but if append needs a larger space it'll make a dynamic memory allocation to support it.

Setting the initial array capacity to stringEncodedLength is just an optimization for the use case that we have.

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

It doesn't change the original behavior, bq here is used as the initial backend array, but if append needs a larger space it'll make a dynamic memory allocation to support it.

Setting the initial array capacity to stringEncodedLength is just an optimization for the use case that we have.

bs = quotient
}
return res
reverse(dst)

This comment has been minimized.

@rbranson

rbranson Jun 9, 2017

Contributor

No way to avoid having to go back through the slice?

@rbranson

rbranson Jun 9, 2017

Contributor

No way to avoid having to go back through the slice?

This comment has been minimized.

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

None that I know of... but I think it's worth it, in the case of a KSUID it'll do 13 loop iterations, which isn't too bad 😛

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

None that I know of... but I think it's worth it, in the case of a KSUID it'll do 13 loop iterations, which isn't too bad 😛

func base2base(in []byte, inBase int, outBase int) []byte {
res := []byte{}
bs := in[:]
func appendBase2Base(dst []byte, src []byte, inBase int, outBase int) []byte {

This comment has been minimized.

@rbranson

rbranson Jun 9, 2017

Contributor

Curios: If the return is dst that has had append() ran on it (presumably causing mutation and a new "value"), how would this save allocations? Is this append() slice magic?

@rbranson

rbranson Jun 9, 2017

Contributor

Curios: If the return is dst that has had append() ran on it (presumably causing mutation and a new "value"), how would this save allocations? Is this append() slice magic?

This comment has been minimized.

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

Yes, this is some append magic. A slice tracks the lengths and the capacity of the backend array it points to, if there's room it'll use it and won't reallocate memory.

More details here => https://blog.golang.org/slices

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

Yes, this is some append magic. A slice tracks the lengths and the capacity of the backend array it points to, if there's room it'll use it and won't reallocate memory.

More details here => https://blog.golang.org/slices

return appendBase2Base(nil, src, inBase, outBase)
}
func appendEncodeBase62(dst []byte, src []byte) []byte {

This comment has been minimized.

@rbranson

rbranson Jun 9, 2017

Contributor

Changing this to work on []byte + adding the []byte Append function to the external interface is actually a pretty big change. This is because this patch makes a subtle semantic modification: the base2base function is purely mathematical, so it works on integer data. In the existing code, the transformation from the integer form to characters in string form abides by the semantic rules of strings. bytestrings don't have encoding metadata, where-as a string does (always UTF-8 in Go!). A rune is not semantically the same as a byte, and a string is not semantically the same as a []byte.

Even though it's convenient that base62 happens to fit into the []byte range of the UTF-8 character set, this causes an implicit coupling between the external Append() method, it's users, and the internal library functions. The problem is pretty benign internally, but much more acute externally as it "leaks" this implicit coupling out through Append(). The only way to use the []byte returned from Append() is as a UTF-8 encoded string, but this is entirely implicit and there are no guard rails.

Thus this long-winded explanation means that I am -1 on exposing a UTF-8 string as []byte through the external interface. Use cases which are performance-sensitive should use the binary representation. The binary representation will always be MUCH faster than the encoded version. I could be swayed from this if we can point to an existing convention/pattern within Go where passing strings as []byte outside of cases that are (semantically) arbitrary bytestrings in the first place.

@rbranson

rbranson Jun 9, 2017

Contributor

Changing this to work on []byte + adding the []byte Append function to the external interface is actually a pretty big change. This is because this patch makes a subtle semantic modification: the base2base function is purely mathematical, so it works on integer data. In the existing code, the transformation from the integer form to characters in string form abides by the semantic rules of strings. bytestrings don't have encoding metadata, where-as a string does (always UTF-8 in Go!). A rune is not semantically the same as a byte, and a string is not semantically the same as a []byte.

Even though it's convenient that base62 happens to fit into the []byte range of the UTF-8 character set, this causes an implicit coupling between the external Append() method, it's users, and the internal library functions. The problem is pretty benign internally, but much more acute externally as it "leaks" this implicit coupling out through Append(). The only way to use the []byte returned from Append() is as a UTF-8 encoded string, but this is entirely implicit and there are no guard rails.

Thus this long-winded explanation means that I am -1 on exposing a UTF-8 string as []byte through the external interface. Use cases which are performance-sensitive should use the binary representation. The binary representation will always be MUCH faster than the encoded version. I could be swayed from this if we can point to an existing convention/pattern within Go where passing strings as []byte outside of cases that are (semantically) arbitrary bytestrings in the first place.

This comment has been minimized.

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

The Append method is a very frequent optimization found in the standard library, which is why I don't feel too bad about it, and the upsides are great when it comes to memory efficiency.

Examples:
=> https://golang.org/pkg/strconv/#AppendBool
=> https://golang.org/pkg/time/#Time.AppendFormat
=> https://golang.org/pkg/math/big/#Int.Append

Yes, the consumer of Append has to understand that it's dealing with UTF-8 encoded data within the byte slice, but this is intended to be used as an optimization when building large strings, most of the code out there will keep using String as a way to get a string representation of the KSUID.

About the internal changes, I agree, it relies on the assumption that the character set we deal with here is all ASCII, which I think is OK since KSUIDs are represented in base 62. If that was to change we would break all code depending on it out there, and if we wanted to introduce alternative representation that use characters out of the ASCII space it would be a good time to revisit this code (although the likeliness makes me feel like the optimization is still worth it).

@achille-roussel

achille-roussel Jun 9, 2017

Contributor

The Append method is a very frequent optimization found in the standard library, which is why I don't feel too bad about it, and the upsides are great when it comes to memory efficiency.

Examples:
=> https://golang.org/pkg/strconv/#AppendBool
=> https://golang.org/pkg/time/#Time.AppendFormat
=> https://golang.org/pkg/math/big/#Int.Append

Yes, the consumer of Append has to understand that it's dealing with UTF-8 encoded data within the byte slice, but this is intended to be used as an optimization when building large strings, most of the code out there will keep using String as a way to get a string representation of the KSUID.

About the internal changes, I agree, it relies on the assumption that the character set we deal with here is all ASCII, which I think is OK since KSUIDs are represented in base 62. If that was to change we would break all code depending on it out there, and if we wanted to introduce alternative representation that use characters out of the ASCII space it would be a good time to revisit this code (although the likeliness makes me feel like the optimization is still worth it).

This comment has been minimized.

@rbranson

rbranson Jun 12, 2017

Contributor

Ok, I'm pretty sold by the standard library uses of it.

@rbranson

rbranson Jun 12, 2017

Contributor

Ok, I'm pretty sold by the standard library uses of it.

@achille-roussel

This comment has been minimized.

Show comment
Hide comment
@achille-roussel

achille-roussel Jun 11, 2017

Contributor

I added another optimization for the ksuid.NewRandom method to keep a memory pool of payload buffers so they don't need to be reallocated on the heap for every call.

  • Before:
$ go test -v -run _ -bench New -benchmem -memprofile /tmp/mem
BenchmarkNew-4   	 1000000	      1243 ns/op	      16 B/op	       1 allocs/op
PASS
ok  	github.com/segmentio/ksuid	1.265s
  • After:
$ go test -v -run _ -bench New -benchmem -memprofile /tmp/mem
BenchmarkNew-4   	 1000000	      1207 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/segmentio/ksuid	1.228s
Contributor

achille-roussel commented Jun 11, 2017

I added another optimization for the ksuid.NewRandom method to keep a memory pool of payload buffers so they don't need to be reallocated on the heap for every call.

  • Before:
$ go test -v -run _ -bench New -benchmem -memprofile /tmp/mem
BenchmarkNew-4   	 1000000	      1243 ns/op	      16 B/op	       1 allocs/op
PASS
ok  	github.com/segmentio/ksuid	1.265s
  • After:
$ go test -v -run _ -bench New -benchmem -memprofile /tmp/mem
BenchmarkNew-4   	 1000000	      1207 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/segmentio/ksuid	1.228s
Show outdated Hide outdated base62.go Outdated

@achille-roussel achille-roussel merged commit 6995043 into master Jun 13, 2017

@achille-roussel achille-roussel deleted the optimizations branch Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment