-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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 zeropool.Pool to workaround SA6002 #12189
Conversation
I built a tiny library called https://github.com/colega/zeropool to workaround the SA6002 staticheck issue. While searching for the references of that SA6002 staticheck issues on Github first results was Prometheus itself, with quite a lot of ignores of it. This changes the usages of `sync.Pool` to `zeropool.Pool[T]` where a pointer is not available. Also added a benchmark for HeadAppender Append/Commit when series already exist, which is one of the most usual cases IMO, as I didn't find any. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
https://www.youtube.com/watch?v=PAAkCSZUG1c&t=9m28s Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
IDK why Windows tests always fail in my PRs, but this is ready for review. |
I don't depend on testify in my lib, but here we have it available. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, I love it. Thanks for this PR. I kind of reviewed your PR on Twitter 🙃
LGTM, I love the results, but even more the READABILITY of the resulting code.
EDIT: I started to write why we should not import your module, but rather copy those 200 locs given the Go proverb A little copying is better than a little dependency., but you already suggested that 😱 Thanks for this, amazing work.
Nice work! |
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
I have pushed two more commits here:
@bwplotka I'd appreciate one more check of those. BenchmarkRangeQuery results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work!
RangeQuery result on CPU is mixed bug, but actually surprisingly good on allocs, great job!
Same as prometheus#12189 but for tsdb/agent/db.go Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Same as #12189 but for tsdb/agent/db.go Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
I built1 a tiny library called zeropool to workaround the SA6002 staticheck issue.
While searching for the references of that SA6002 staticheck issues on Github first results was Prometheus itself, with quite a lot of ignores of it.
This changes the usages of
sync.Pool
tozeropool.Pool[T]
where a pointer is not available (some usages likememChunkPool
do use thesync.Pool
properly, and changing those would make performance worse).Also added a benchmark for
HeadAppender.{Append,Commit}
for a scenario when series already exist, which is one of the most usual cases IMO, as I didn't find any.I think the code is cleaner now, and the benchmarks results are quite promising:
Footnotes
Shameless self plug. ↩