Skip to content
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

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

Merged
merged 9 commits into from
Aug 3, 2022

Conversation

robert-zaremba
Copy link
Collaborator

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.

Copy link
Member

@tac0turtle tac0turtle left a comment

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
Collaborator 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
Collaborator Author

turns out that it's significant optimization

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

@p0mvn p0mvn left a comment

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

CHANGELOG.md Outdated Show resolved Hide resolved
// 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
Member

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
Collaborator 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
Member

Choose a reason for hiding this comment

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

@@ -95,14 +97,14 @@ func (nc *lruCache) Len() int {
}

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
Member

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
Member

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

Copy link
Collaborator Author

@robert-zaremba robert-zaremba Aug 2, 2022

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
Collaborator 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)

Copy link
Member

@p0mvn p0mvn left a comment

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants