search

prometheus / alertmanager

Prometheus Alertmanager
https://prometheus.io
Apache License 2.0
6.69k stars 2.17k forks source link

replace Resolved with ResolvedAt to reduce amount call to time.Now() #4119

Open evkuzin opened 1 week ago

evkuzin commented 1 week ago

with big number of inhibitions I see pretty bad picture when we spend plenty of time in time.Now() This can be fixed by calling it once per hasEqual

image
evkuzin commented 1 week ago

@w0rm WDYT?

grobinson-grafana commented 1 week ago

Seems reasonable to me. What does the profile look like with the change?

evkuzin commented 1 week ago

@grobinson-grafana thx for taking a look! attaching a picture from pyroscope. On the left is a binary with a patch and on the right second node from the same cluster

image
evkuzin commented 1 week ago

I suppose we can go further and replace this comparison https://github.com/prometheus/alertmanager/pull/4119/files#diff-279dfe3df5d26e996ad18fe4a97c9dea0c7c4d5079c11c96d4a4644e942602dfL240 with something better. We already using FNV-1A fingerprint and comparing uint64 instead of strings would dramatically improve this other bit where we waste tonn of time (runtime.mapaccess1_faststr ). But for now I just want this quick win.

evkuzin commented 1 week ago

Regarding the failed test. it seems like this is not enough for alertmannger to start on CI? Do I need to fix it?

grobinson-grafana commented 1 week ago

I would benchmark the difference between mapaccess1_faststr and mapaccess1_fast64 to be sure there is a considerable speed up, as from what I can tell fast64 also calculates hashes on the int64. It might be that fnv1 + fast64 is slower than faststr.

evkuzin commented 1 week ago

fair enough. Anyway, I didnt plan to include this in the current PR. Does this look like it can be accepted by any chance?

evkuzin commented 5 days ago

@SuperQ can it be merged? Whats the process?

SuperQ commented 4 days ago

I would like one of the Alertmanager maintainers to look at this before merging.