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

Begin v2 of the API, fixes memory usage #139

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Begin v2 of the API, fixes memory usage #139

wants to merge 2 commits into from

Conversation

evanphx
Copy link
Contributor

@evanphx evanphx commented Mar 5, 2024

While hunting down some memory usage issues elsewhere, I saw that, much to my chagrin, hclog was suffering from the exact memory issue issue that log/slog was designed to avoid: memory usage when the log is ignored.

As Jonathan Amsterdam points out extremely well in his presentations of slog, Go is unable to elide a ... generated slice when calling an interface function. This makes completely sense, given it has to assume it could dispatch to any number of targets.

Following in the same pattern used in slog, this PR begins a minor (but breaking) API change to correct the issue in hclog. By making Logger a struct value (that can be used as a pointer or not, it's no matter) and performing level checking before dispatching to the interface, the memory usage is elided correctly.

The benchmark included shows the memory fix as it now reports:

BenchmarkLoggerMemory-10        250033984                5.027 ns/op           0 B/op          0 allocs/op

Where as previously, it reports:

BenchmarkLoggerMemory-10        40523308                30.87 ns/op           64 B/op          1 allocs/op

There is more work to be done if we approve of this going forward, for instance, using the same technique slog uses to clean up the line calculation.

Here is Jonathan talking all about the issue that this v2 API would fix: https://youtu.be/8rnI2xLrdeM?t=1491

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.

1 participant