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.

Sign up for GitHub

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

Jump to bottom

perf: "app." + key is allocating new objects still #401

Closed
lizthegrey opened this issue Nov 22, 2023 · 1 comment · Fixed by #406
Closed

perf: "app." + key is allocating new objects still #401

lizthegrey opened this issue Nov 22, 2023 · 1 comment · Fixed by #406
Assignees
@MikeGoldsmith
Labels
type: bug Something isn't working

Comments

@lizthegrey
Copy link
Member

Versions

  • Go: 1.21.4
  • Beeline: 1.13.0

Steps to reproduce

  1. Run shepherd at high volume ;)
  2. Look at the profiles.
  3. Weep.

Screenshot from 2023-11-22 10-04-31

Additional context
#328 tried to make this better (before, we were using sprintf which is worse ugh), but it's now 7% of our allocations. This is not good. We probably would save time using a LRU cache for mapping foo to app.foo to avoid allocating brand new strings every time.
@lizthegrey lizthegrey added the type: bug Something isn't working label Nov 22, 2023
@MikeGoldsmith MikeGoldsmith mentioned this issue Nov 27, 2023
Closed
@MikeGoldsmith MikeGoldsmith self-assigned this Nov 27, 2023
@robbkidd robbkidd mentioned this issue Nov 28, 2023
Merged
@robbkidd
Copy link
Member

robbkidd commented Nov 28, 2023 •
edited
Loading

Another contender for a memory improvement when adding fields: #406 or "allow the user to optimize".

robbkidd added a commit that referenced this issue Nov 30, 2023
@robbkidd @JamieDanielson
perf: speed up adding fields, reduce memalloc if field name is alread…
644691f 
…y prefixed with "app." (#406)

## Which problem is this PR solving?

- Closes #401 

There is a remarkable amount of memory allocation occurring under load
to perform the "app." prefixing of field names by the Beeline.

## Short description of the changes

This change skips the memory allocations needed for string concat and
usage if the "app." prefix is already present on the field name
provided.

* doc comments updated to recommend users prefix their field names
  with "app." when calling these beeline.AddField*() functions
* doc comments also updated to make the handling of Error as value
  more clear

### Benchmarks

Existing behavior is no slower or memory hungry, but for every field
name provided by the Beeline user that starts with "app.", that is one
less memory allocation of the size of the field name string.

```
BenchmarkBeelineAddField/oldAddField/whatever
BenchmarkBeelineAddField/oldAddField/whatever-12                19654003                60.52 ns/op            8 B/op          1 allocs/op
BenchmarkBeelineAddField/AddField/no-prefix
BenchmarkBeelineAddField/AddField/no-prefix-12                  18939754                60.65 ns/op            8 B/op          1 allocs/op
BenchmarkBeelineAddField/AddField/half-prefixed
BenchmarkBeelineAddField/AddField/half-prefixed-12              22405790                51.22 ns/op            4 B/op          0 allocs/op
BenchmarkBeelineAddField/AddField/all-prefixed
BenchmarkBeelineAddField/AddField/all-prefixed-12               27381916                42.88 ns/op            0 B/op          0 allocs/op
```

---------

Co-authored-by: JamieDanielson <jamieedanielson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MikeGoldsmith MikeGoldsmith

Labels
type: bug Something isn't working
Projects
None yet
Milestone
No milestone
3 participants
@robbkidd @lizthegrey @MikeGoldsmith