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

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

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

Conversation

evkuzin
Copy link

@evkuzin evkuzin commented Nov 13, 2024

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 evkuzin force-pushed the fix_time_now_enormous_overhead branch 2 times, most recently from bfe9b6b to 7f0c30b Compare November 13, 2024 09:19
…me.Now() to 1 per iteration

Signed-off-by: Evgeny Kuzin <evgeny.kuzin@outlook.com>
@evkuzin evkuzin force-pushed the fix_time_now_enormous_overhead branch from 7f0c30b to 7f5c8ff Compare November 13, 2024 09:21
@evkuzin
Copy link
Author

evkuzin commented Nov 13, 2024

@w0rm WDYT?

@grobinson-grafana
Copy link
Contributor

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

@evkuzin
Copy link
Author

evkuzin commented Nov 15, 2024

@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
Copy link
Author

evkuzin commented Nov 15, 2024

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
Copy link
Author

evkuzin commented Nov 15, 2024

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
Copy link
Contributor

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
Copy link
Author

evkuzin commented Nov 15, 2024

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

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