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

pre-define map capacities #197

Merged
merged 1 commit into from
Oct 17, 2022
Merged

pre-define map capacities #197

merged 1 commit into from
Oct 17, 2022

Conversation

lizthegrey
Copy link
Member

Which problem is this PR solving?

  • Excess time spent in github.com/honeycombio/libhoney-go.(*Builder).NewEvent according to profiles.

Short description of the changes

  • Takes locks earlier before creating the map, so that the size of the map can be known before populating it.

@lizthegrey lizthegrey requested review from dstrelau, a team and kentquirk October 12, 2022 20:58
@vreynolds vreynolds added the status: oncall Flagged for awareness from Honeycomb Telemetry Oncall label Oct 14, 2022
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.

Looks good, thanks @lizthegrey 👍🏻

@MikeGoldsmith MikeGoldsmith merged commit e707bf5 into main Oct 17, 2022
@MikeGoldsmith MikeGoldsmith deleted the lizf.optimise-maps branch October 17, 2022 13:52
MikeGoldsmith pushed a commit to honeycombio/beeline-go that referenced this pull request Oct 17, 2022
## Which problem is this PR solving?

- Similar to honeycombio/libhoney-go#197, profiling shows we spend a lot of time in github.com/honeycombio/beeline-go/trace.(*Span).send generating the map.

## Short description of the changes

- define the map capacity in advance before returning it.

a subsequent follow-up might be allowing passing a map in its entirety to github.com/honeycombio/libhoney-go.(*Event).AddField so that the map entries do not need to be added one at a time when merging the maps together.

Or, better yet, golang/go#56182 could prevent this problem by giving us a more sensible API for mass additions to a map.
kentquirk added a commit that referenced this pull request Oct 17, 2022
kentquirk added a commit that referenced this pull request Oct 17, 2022
Reverts #197

The solution here adds additional locks and lacks configurability, and
we'd like to first try a different path. A combination of vacation, time
zones, and a weekend caused our wires to get crossed.
@MikeGoldsmith MikeGoldsmith added version: bump patch A PR with release-worthy changes and is backwards-compatible. and removed status: oncall Flagged for awareness from Honeycomb Telemetry Oncall labels Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version: bump patch A PR with release-worthy changes and is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants