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

Use sync.Pool and bytes.Buffer for Logger.With perf improvement #594

Closed
wants to merge 1 commit into from

Conversation

a-menshchikov
Copy link

There are currently a lot of allocations in the Logger.With due to direct make calls. This PR replaces it by using sync.Pool and bytes.Buffer.

@rs
Copy link
Owner

rs commented Oct 9, 2023

Thanks, I'll check the PR. Just curious, why are you calling With so often? The goal of With is to call it once for a logger.

@a-menshchikov
Copy link
Author

@rs I have the helper function that gets a logger from context and adds some trace attributes (trace id and span id) on each call. It brings some overhead, but looks good for me and allows my team to write simpler code.

P.S. I see data races, now I try to solve this issue.

@a-menshchikov
Copy link
Author

@rs it seems I don't know how to fix data races, which occured by simultaneously use of -race and -bench. =(

Could you suggest some solution?

@rs
Copy link
Owner

rs commented Oct 9, 2023

Your implementation is actually not correct as the buffer is used after you put it back in the pool. If you want to pull the buffer, you need to pull it in With and put it back once the logger is no longer used. With the current API, there is no way you could implement such pooling.

@a-menshchikov
Copy link
Author

@rs thank you for clarifying, you are absolutely right, it's my fail. I have tried several ways to work around this problem, but it seems like I need to use Event.Fields instead of Logger.With to solve my problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants