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

Code optimization: The relabel operation is used very frequently, and strconv.FormatInt() with better performance should be used. #13669

Merged
merged 1 commit into from
May 14, 2024

Conversation

aiwhj
Copy link
Contributor

@aiwhj aiwhj commented Feb 29, 2024

Code optimization: The relabel operation is used very frequently, and strconv.FormatInt() with better performance should be used.
Fixed #13668

… strconv.FormatInt() with better performance should be used.

Signed-off-by: roger.wang <roger.wang@cloudwise.com>
@machine424
Copy link
Collaborator

even if it seems trivial to you, adding some benchmarks may help justify this.

@aknuds1
Copy link
Contributor

aknuds1 commented Apr 5, 2024

Agree with @machine424.

Copy link
Contributor

@tesla59 tesla59 left a comment

Choose a reason for hiding this comment

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

The difference is very minimal

goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/model/relabel
                     │   fmt.txt   │             strconv.txt             │
                     │   sec/op    │   sec/op     vs base                │
Relabel/example-8      1.211µ ± 0%   1.210µ ± 0%       ~ (p=0.350 n=100)
Relabel/kubernetes-8   5.432µ ± 0%   5.358µ ± 0%  -1.35% (n=100)
geomean                2.564µ        2.546µ       -0.70%

                     │   fmt.txt    │             strconv.txt              │
                     │     B/op     │     B/op      vs base                │
Relabel/example-8        606.0 ± 0%     606.0 ± 0%   0.00% (p=0.010 n=100)
Relabel/kubernetes-8   1.417Ki ± 0%   1.417Ki ± 0%  -0.03% (p=0.000 n=100)
geomean                  937.7          937.6       -0.02%

                     │  fmt.txt   │             strconv.txt              │
                     │ allocs/op  │ allocs/op   vs base                  │
Relabel/example-8      19.00 ± 0%   19.00 ± 0%       ~ (p=1.000 n=100) ¹
Relabel/kubernetes-8   36.00 ± 0%   36.00 ± 0%       ~ (p=1.000 n=100) ¹
geomean                26.15        26.15       +0.00%
¹ all samples are equal

Number of test samples: 100
Run on MacBook Air M2 with 16GB of RAM and MacOS 14.4.1

@tesla59
Copy link
Contributor

tesla59 commented May 5, 2024

Although I think using strconv would be idiomatic because it is more readable

@machine424
Copy link
Collaborator

Maybe we should enforce this using a linter or sth.
There is golangci/golangci-lint#3714
and #13220 tried to do that.

@alexandear
Copy link
Contributor

I created PR #14091, in which I made the same changes and enabled the perfsprint linter to prevent similar situations in the future.

@tesla59
Copy link
Contributor

tesla59 commented May 13, 2024

Maybe we should enforce this using a linter or sth. There is golangci/golangci-lint#3714 and #13220 tried to do that.

As per your suggestions, i have created the pr #14092 which enables the linter and fixed all issues generated by it

@machine424
Copy link
Collaborator

Thanks, @tesla59, for your responsiveness and efforts. However, I’m afraid @alexandear re-opened his PR first. He’s the author of the closed PR I linked to, where we discussed the reopening. You could help us review it if you want.
(To avoid such conflicts in the future, I’d suggest expressing your intention to work on an issue before creating a PR ;))

@tesla59
Copy link
Contributor

tesla59 commented May 13, 2024

Thanks, @tesla59, for your responsiveness and efforts. However, I’m afraid @alexandear re-opened his PR first. He’s the author of the closed PR I linked to, where we discussed the reopening. You could help us review it if you want.
(To avoid such conflicts in the future, I’d suggest expressing your intention to work on an issue before creating a PR ;))

Sure @machine424 I'll make sure to comment before working to avoid such conflicts

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks!

@bboreham bboreham merged commit dc92652 into prometheus:main May 14, 2024
24 checks passed
krajorama added a commit to grafana/mimir that referenced this pull request May 15, 2024
krajorama added a commit to grafana/mimir that referenced this pull request May 15, 2024
grafanabot pushed a commit to grafana/mimir that referenced this pull request May 15, 2024
* Update mimir-prometheus to e5b85c151ba8

grafana/mimir-prometheus@e5b85c1

prometheus/prometheus#14103
prometheus/prometheus#14096
prometheus/prometheus#14068
prometheus/prometheus#13669

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
(cherry picked from commit 307567a)
krajorama added a commit to grafana/mimir that referenced this pull request May 15, 2024
* Update mimir-prometheus to e5b85c151ba8

grafana/mimir-prometheus@e5b85c1

prometheus/prometheus#14103
prometheus/prometheus#14096
prometheus/prometheus#14068
prometheus/prometheus#13669

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
(cherry picked from commit 307567a)

Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
francoposa pushed a commit to grafana/mimir that referenced this pull request May 27, 2024
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.

strconv will be better to fmt
6 participants