Skip to content

Add TagReplacingFilter #6183

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

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

Conversation

etki
Copy link

@etki etki commented Apr 26, 2025

Hey. This is a PR aimed at performance. Currently, MeterFilter.replaceTagValues uses an anonymous class with several drawbacks:

  • It uses a stream, which results in a lot of allocations, for a collection with a handful of elements. Usually we don't care about that much when it happens inside some boring web request processing of twenty elements on a page, but micrometer is a part of a critical infrastructure that might be call hundreds of time more often.
  • There is an inner loop which results in O(n²) processing
  • It is collected to a list, which is created empty and is dynamically resized afterwards.
  • The identifier is always recreated (= +1 allocation) even in the case no tags were actually modified
  • The anonymous class is represented as $ in stacktraces, which makes it hard to analyze (this is a part of the story how i got here)

PR addresses these issues and wins some performance. As with another PR, implementing suggestions in #6113 may easily boost wins by a magnitude. Benchmarks from #6174, OpenJDK 21.0.2 and Intel N100 fixed at 2GHz were used to evaluate the impact. The benchmark was fed with identifiers having 0-64 tags, 90% of time having exactly mode tags, and any other number with even probability otherwise. Please be aware that these results include some of the updates to Tags from a forthcoming PR; i might be able to re-run the benchmarks without Tags updates, but i'm not sure i'll have the time.

mode version ns/op instructions/op
0 main 233.983 ± 0.485 1398.565
0 PR 85.120 ± 5.041 459.668
1 main 285.697 ± 0.620 1642.970
1 PR 125.450 ± 0.754 642.829
2 main 353.423 ± 1.433 1965.876
2 PR 145.541 ± 5.641 777.018
4 main 411.847 ± 0.868 2390.004
4 PR 177.064 ± 1.155 904.484
8 main 570.672 ± 1.713 3151.117
8 PR 244.465 ± 1.372 1241.124
16 main 916.877 ± 0.992 4890.982
16 PR 417.049 ± 2.131 1902.963
32 main 1587.275 ± 1.791 8183.333
32 PR 869.795 ± 15.653 4312.889
64 main 2208.968 ± 12.309 14079.874
64 PR 1640.243 ± 33.729 9516.821

Please note that of course for 0 tags it would take less than 10ns, and other lower amounts would also have lower processing time - it's the fact that 10% of identifiers in the sample set possess 0-64 tags and drag the time higher by that.

@etki etki force-pushed the tag-replacing-filter branch from 16cea7d to 6eb2663 Compare April 26, 2025 09:13
while (iterator.hasNext()) {
Tag tag = iterator.next();
String key = tag.getKey();
String value = tag.getValue();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of hacky move that can be hindering performance instead of helping it. The idea is simple - since predicate and replacing function aren't really interested in the tag itself, only in the key and value, let's extract them once, store in registers, and prevent double memory access. This sounds good, but in reality it might end up with two additional stack writes (which are relatively cheap and wouldn't make that much difference): if for any reason compiler can't fully inline the predicate and function, it will have to store the registers on the stack, and instead of one tag reference it will be forced to store one tag and two string references. However, this still saves us one indirection.

TBH this is solved simply by running a pair of benchmarks, but i'm all out of energy, and there are a couple more PRs coming

@etki etki force-pushed the tag-replacing-filter branch 4 times, most recently from fccafc5 to 4de70ed Compare April 26, 2025 09:54
@etki etki force-pushed the tag-replacing-filter branch from 4de70ed to 8364f76 Compare April 26, 2025 09:55

return id.replaceTags(tags);
}
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What were the problems / what was solved:

  • Stream was quite allocateful for a handful of elements. We're usually OK for that in writing our applications, but for critical paths we'd like to avoid that
  • for (String exception : exceptions) was adding O(n²) complexity
  • toList() creates a new zero-sized list. This results in (usually) a number of unnecessary resizes.
  • Finally, we'd like to abstain from recreating meter if there were no actual replacements. This doesn't do really much, but just a pleasant addition + even if we've allocated a new list, there is no record in GC card table pointing at it.

@jonatan-ivanov jonatan-ivanov added the performance Issues related to general performance label Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues related to general performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants