Skip to content

perf: use fast unsafe bytes->string convertion#525

Merged
tac0turtle merged 9 commits into
masterfrom
robert/string-conv
Aug 3, 2022
Merged

perf: use fast unsafe bytes->string convertion#525
tac0turtle merged 9 commits into
masterfrom
robert/string-conv

Conversation

@robert-zaremba

Copy link
Copy Markdown
Contributor

Avoid unnecessary allocation by using fast unsafe bytes -> string conversion

Notes:

  • I didn't add conversions to the tests - I'm planning other PR there to cleanup key / values repetitions.

@tac0turtle tac0turtle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you provide some before and after benchmarks to verify the improvements

@robert-zaremba

Copy link
Copy Markdown
Contributor Author

Benchmark:

benchmark                                old ns/op     new ns/op     delta
BenchmarkGetNonMembership/fast-16        143368        129285        -9.82%
BenchmarkGetNonMembership/regular-16     10305208      8466036       -17.85%
BenchmarkImmutableAvlTreeMemDB-16        150397        139207        -7.44%

benchmark                             old allocs     new allocs     delta
BenchmarkImmutableAvlTreeMemDB-16     437            392            -10.30%

benchmark                             old bytes     new bytes     delta
BenchmarkImmutableAvlTreeMemDB-16     33513         32100         -4.22%

@robert-zaremba

Copy link
Copy Markdown
Contributor Author

turns out that it's significant optimization

@tac0turtle tac0turtle requested a review from p0mvn August 1, 2022 14:40

@p0mvn p0mvn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @robert-zaremba . Great work on this optimization. I had a couple of questions. Please let me know what you think

Comment thread CHANGELOG.md Outdated
Comment thread internal/bytes/string.go
// UnsafeStrToBytes uses unsafe to convert string into byte array. Returned bytes
// must not be altered after this function is called as it will cause a segmentation fault.
func UnsafeStrToBytes(s string) []byte {
var buf []byte

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about the following to avoid copying the slice internals by directly casting the string:

func unsafeGetBytes(s string) []byte {
    return (*[0x7fff0000]byte)(unsafe.Pointer(
        (*reflect.StringHeader)(unsafe.Pointer(&s)).Data),
    )[:len(s):len(s)]
}

I'm not sure if there are any constraints preventing that but this could be a further optimization

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What is [0x7fff0000]byte? Looks like a big static array. Where did you get that solution from?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread cache/cache.go

func (c *lruCache) Remove(key []byte) Node {
if elem, exists := c.dict[string(key)]; exists {
if elem, exists := c.dict[ibytes.UnsafeBytesToStr(key)]; exists {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm surprised that this is helpful because, from my understanding, copying during []byte to string conversion when accessing a map by key should be optimized by the Go compiler.

Sources:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the benchmark would remain the same as it is right now if the map[string] changes are reverted

@robert-zaremba robert-zaremba Aug 2, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can check, however for consistency I prefer to keep the casting here.

Co-authored-by: Roman <ackhtariev@gmail.com>
@robert-zaremba

Copy link
Copy Markdown
Contributor Author

After removing the fast convertion, there is a slight performance degradation:

  • before = with unsafe string in cache.go maps
  • after = with normal string(key) lookup in cache.go maps.
benchmark                             old ns/op     new ns/op     delta
BenchmarkImmutableAvlTreeMemDB-16     138184        140417        +1.62%

benchmark                             old allocs     new allocs     delta
BenchmarkImmutableAvlTreeMemDB-16     392            419            +6.89%

benchmark                             old bytes     new bytes     delta
BenchmarkImmutableAvlTreeMemDB-16     32154         32928         +2.41%

I repeated it few times and the memory allocation results were similar (there was more variation in the ns/op)

@p0mvn p0mvn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks for trying the map benchmarks.

Let's go with the approach that benchmarks show is best.

We can investigate this further separately

@tac0turtle tac0turtle merged commit 3e26f26 into master Aug 3, 2022
@tac0turtle tac0turtle deleted the robert/string-conv branch August 3, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants