search

rs / zerolog

Zero Allocation JSON Logger
MIT License
10.39k stars 566 forks source link

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

Closed a-menshchikov closed 11 months ago

a-menshchikov commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

@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.