-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
NormalizeMetricFamilies: Optimize memory usage #1394
base: main
Are you sure you want to change the base?
Conversation
Hey, thanks for the PR! Could you sign the commits to make CI happy? I'm assuming the efficiency improvement here is that we don't need to allocate memory for metric families with empty metric arrays, but could you elaborate if that's really what you're improving here? |
Signed-off-by: ethanvc <xht_xht@qq.com>
955a5b3
to
6f666c7
Compare
Sorry, already signed off.
But i am not too sure if the key for |
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.
By creating result
before pruning metricFamilies with empty metrics, we might be over allocating memory 🤔
Not allocating space for names looks more relevant though, so that's good :)
But i am not too sure if the key for
metricFamiliesByName
is always equal toMetricFamily.Name
, which may crash if Name is nil after this change.
Reading the code now, I can see that we get the metricFamily name from desc.fqName, which can't be nil or empty(See validation[1][2])
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 @ethanvc ! BTW what triggered this change? Some production performance issue or just looking at the code?
Generally, it's a premature optimization if we don't have a memory profile suggesting this code path inefficiency matters. It's also might be very ineffective change if we don't do any (at least) microbenchmarks to verify if we worsened or improved the efficiency (learning from my "Efficient Go" book btw) (:
With that preambule, let's check if, at least in theory, this makes sense:
- do not need to allocate memory for names, which is a little large when we have lots of metrics.
Makes sense.
- do not need to allocate memory for empty metrics, but i think we may have very small amount of them.
Your code still allocates memory for empty metrics, but indeed that should be fine. I don't know of any case where metric family might have empty metric (there might be, just I might not educated here) 🤔
- do not need to searh the map for every mertrics, which save cpu times.
Not sure what do you mean by search, maybe you meant sort? I assume mf.Metric == 0 case is near 0% often, so on scale I don't think it matters. Generally you reduced from having 3 for loops to 1 for loop, but those are so small I doubt we will see considerable or any CPU improvement in practice. This is where only good benchmark would tell us.
But i am not too sure if the key for metricFamiliesByName is always equal to MetricFamily.Name, which may crash if Name is nil after this change.
Reading the code now, I can see that we get the metricFamily name from desc.fqName, which can't be nil or empty(See validation[1][2])
Nice, I am not worried about empty or nil case (see my suggestion). I more worried about map key is different than mf.Name. As @ArthurSens verified, this should be the case (for now 🙈 ).
Anyway, I think it's little harm to merge this (with my suggestion) WITHOUT micro-benchmarks, but I wouldn't be too excited here. I would even be not surprised if this function is now slower due to things we might miss (cacheability of the code, branching etc). Making any assumptions on such low level is guessing (:
It should allocate less (without benchmark that's again, just guess), but is it a lot? Even with 1000 metric families (extreme already) for []string that's only 24 + 16*1k bytes, so ~16KB of memory on every scrape (10-60s), easily garbage collected in between (I don' think this function is used more often)
LGTM, with below suggestion to be fixed first. Thanks @ArthurSens for detailed review 💪🏽
my first contribution to prometheus and this mr is going to make code more efficient.