You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Please describe the feature request.
Hi. As you surely know, all observability instruments have more demands for performance than anything else, as they lie on the critical path and are called more often than anything. Currently, Tags and its surroundings are major performance drainers due to many unoptimized places (i bet you've faced it, and it was the reason for Tags#merge()), and due to its API i can't solve many problems myself without overriding the class and its siblings in my own codebase. My suggestions are the following:
Add Tags#length() and Tags#isEmpty() methods. Tags is already an Iterable and streamable type, there's not much sense in not doing it. With it, consumers would be able to size their ArrayList's and arrays properly, should they need to do some kind of processing.
Add Tags#getTag(int i) method. This would allow zero-allocation iterations.
Add Tags#getTag(String key) method and reuse it in Meter#getTag(String key). It would be also beneficial to test if binary search would be more performant on common tag collection sizes.
Add a Tags#asList() method, which will simply return Collections.unmodifiableList(Arrays.asList(content)). This will provide a common interface for consumers with almost minimal possible allocation.
Use it in Meter.Id#getTags(). This will significantly reduce allocation (currently a new ArrayList is allocated there, without a capacity hint, which also adds penalty) and avoid a potential call in currently existing forEach() call (if it won't get inlined).
Expose Tags Meter.Id#getRawTags() or a similar method. There is not much sense in hiding it, as class is immutable and is already returned through getTagsAsIterable().
Expose a method of kind Tags.Unsafe#ofSorted(Tag... tags) to avoid sorting altogether for the people who know what they are doing. I merge different sorted sources of tags in my case, by knowing that i can merge this sources in an already sorted way.
Add Meter.Id#replaceTags(Tags), which would allow to skip excessive calls (at least .iterator().next() is currently on the code path, unnecessary allocation that could have been avoided).
Of course i could have easily submitted a pull request, but i expect these to be design changes that don't go without a discussion beforehand.
Rationale
In my profiles, MeterFilter.commonTags() and MeterFilter.ignoreTags() consume more than actual metric accounting, mostly due to (partially fixed now; i'm facing the issue is on 1.13) Tags.and(), Tags.concat() and so on. Intercepting a potential question, using 1.14 won't solve it, as these still won't be free, and there are more offenders, such as a stream (= a lot of unnecessary allocations) with O(n²) complexity in MeterFilter#ignoreTags(), which in the end creates a re-sorted Tags (and since it is just a punch hole iteration of an already sorted list, another sort isn't needed). I want to provide a PR addressing some aspects of the latter, but it's not ready as of now.
Additional context
Add any other context about the feature request here, e.g. related issues, prior art.
The text was updated successfully, but these errors were encountered:
Please describe the feature request.
Hi. As you surely know, all observability instruments have more demands for performance than anything else, as they lie on the critical path and are called more often than anything. Currently, Tags and its surroundings are major performance drainers due to many unoptimized places (i bet you've faced it, and it was the reason for
Tags#merge()
), and due to its API i can't solve many problems myself without overriding the class and its siblings in my own codebase. My suggestions are the following:Tags#length()
andTags#isEmpty()
methods. Tags is already an Iterable and streamable type, there's not much sense in not doing it. With it, consumers would be able to size their ArrayList's and arrays properly, should they need to do some kind of processing.Tags#getTag(int i)
method. This would allow zero-allocation iterations.Tags#getTag(String key)
method and reuse it inMeter#getTag(String key)
. It would be also beneficial to test if binary search would be more performant on common tag collection sizes.Tags#asList()
method, which will simply returnCollections.unmodifiableList(Arrays.asList(content))
. This will provide a common interface for consumers with almost minimal possible allocation.Meter.Id#getTags()
. This will significantly reduce allocation (currently a new ArrayList is allocated there, without a capacity hint, which also adds penalty) and avoid a potential call in currently existing forEach() call (if it won't get inlined).Tags Meter.Id#getRawTags()
or a similar method. There is not much sense in hiding it, as class is immutable and is already returned through getTagsAsIterable().Tags.Unsafe#ofSorted(Tag... tags)
to avoid sorting altogether for the people who know what they are doing. I merge different sorted sources of tags in my case, by knowing that i can merge this sources in an already sorted way.Meter.Id#replaceTags(Tags)
, which would allow to skip excessive calls (at least .iterator().next() is currently on the code path, unnecessary allocation that could have been avoided).Of course i could have easily submitted a pull request, but i expect these to be design changes that don't go without a discussion beforehand.
Rationale
In my profiles,
MeterFilter.commonTags()
andMeterFilter.ignoreTags()
consume more than actual metric accounting, mostly due to (partially fixed now; i'm facing the issue is on 1.13)Tags.and()
,Tags.concat()
and so on. Intercepting a potential question, using 1.14 won't solve it, as these still won't be free, and there are more offenders, such as a stream (= a lot of unnecessary allocations) with O(n²) complexity inMeterFilter#ignoreTags()
, which in the end creates a re-sorted Tags (and since it is just a punch hole iteration of an already sorted list, another sort isn't needed). I want to provide a PR addressing some aspects of the latter, but it's not ready as of now.Additional context
Add any other context about the feature request here, e.g. related issues, prior art.
The text was updated successfully, but these errors were encountered: