Builder pattern using Tags leads to high CPU overhead

[DanielThomas]

We discovered some significant CPU hotspots in a service and found several places in our frameworks that were doing a significant amount of unnecessary work due to the sort/dedup on every Tags class operation. In our case we were either using Tags as a builder with up to a dozen calls to Tags::and or has several calls to tags(String, String) on the builders:

Appears when this commit landed it switched all builders to useTags internally, so incur the sort/dedup penalty for every call, making building tags quite expensive:

b7bbea3#diff-a64eec4db3aaf5a50e01e46efed55b3eecb772bcbe0ad929ef59b455c7a5ec2c

This makes Tags a poor choice for intermediate Tags state and if sort/dedupe can't be avoided, there should probably be a mutable builder class backing these usages instead. If that overhead can't be avoided, I'd argue that the builder-style methods on Tags should be deprecated, because it's easy to get into trouble and not realise it.

Environment

• Micrometer version 1.11.3

To Reproduce

Use multiple calls to Tags::and and note the overhead.

Expected behavior

Creating multiple tags has as little overhead as possible.

Additional context

Netflix/dgs-framework#1612

[DanielThomas]

I should add, if there's the possibility of avoiding this completely, that'd be great - before these changes, these accounted for 50%(!) of the CPU overhead for the application. We've gotten that down a bunch, but it's unavoidable when doing the final measurement:

[shakuzen]

Thank you for the report and details. It's much appreciated. We would need to look into this carefully. Unfortunately, I think the complete motivation/context in which the original changes were made (almost 5 years ago, for version 1.1.0) is lost at this point. So we'd really need to dig into all things that could be affected by making changes around this.

I should add, if there's the possibility of avoiding this completely, that'd be great

To be clear, you mean avoiding the sort and deduplication?

before these changes, these accounted for 50%(!) of the CPU overhead for the application.

That's unexpected. There is certainly more overhead with it being immutable and sorting/deduping each call to and, but it should still be a rather small overhead in absolute terms. Is the application just not doing much work on the CPU?

[DanielThomas]

Yes, particularly the sort. I didn't dig too far into the history, but it wasn't clear to me why the tags are sorted at this point. Feels like something that should be the concern of the registry if it's required, so we don't pay this overhead where it's not required.

This particular application is a DGS framework application, which has timers around GraphQL field data fetchers, which is what you're seeing in that last screenshot. Those data fetchers also use Spring Security method authorization and we have counters for authz decisions too, so each incoming query will have dozens of metrics recorded.

This service mostly spends most of the time in GRPC doing the data fetching to other queries, which is why instrumentation can be such a CPU hotspot compared to the other work going on.

[jiangxinlingdu]

    private Tags(Tag[] tags) {
        this.tags = tags;
        Arrays.sort(this.tags);
        dedup();
    }

i think sort unnecessary