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: speed up adding fields, reduce memalloc if field name is already prefixed with "app." #406

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

robbkidd
Copy link
Member

Which problem is this PR solving?

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.

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

There is a remarkable amount of memory allocation occurring under load
to perform the field name "app." prefixing by the Beeline with "app.".
This change skips the memory allocations needed for string concat and
usage if the "app." prefix is already present on the field name
provided.
@robbkidd robbkidd requested a review from a team November 28, 2023 23:16
@robbkidd robbkidd self-assigned this Nov 28, 2023
@robbkidd robbkidd added the type: enhancement New feature or request label Nov 28, 2023
beeline_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

This is a good call, and provides a nice on-ramp to adding the app prefix explicitly.

@robbkidd robbkidd changed the title fix: reduce memalloc if field name is already prefixed perf: reduce memalloc if field name is already prefixed Nov 28, 2023
@robbkidd robbkidd added the version: bump minor A PR that adds behavior, but is backwards-compatible. label Nov 29, 2023
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

I think this is the right way to go. The work done in #402 by introducing a cache introduced too much lock contention. This will allow users of the beeline to improve perf. by doing the app. prefix themselves.

The query is not blocking and I don't think it'll have a material impact.

beeline.go Outdated Show resolved Hide resolved
We have concluded that calling functions as parameters to functions is
not as easy to read as local variable assignment and using the variable
as the parameter. Benchmarking indicates that there is no difference in
allocations between assigning a local variable in AddField() and having
a function return a value that is immediately used as a parameter still
within the scope of AddField().
@robbkidd robbkidd changed the title perf: reduce memalloc if field name is already prefixed perf: speed up adding fields and reduce memalloc if field name is already prefixed with "app." Nov 30, 2023
@robbkidd robbkidd changed the title perf: speed up adding fields and reduce memalloc if field name is already prefixed with "app." perf: speed up adding fields, reduce memalloc if field name is already prefixed with "app." Nov 30, 2023
@robbkidd robbkidd merged commit 644691f into main Nov 30, 2023
3 checks passed
@robbkidd robbkidd deleted the robb.reduce-allocs-adding-fields branch November 30, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: "app." + key is allocating new objects still
3 participants