Skip to content

codeclimate: less memory allocation #3882

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 4, 2023
Merged

codeclimate: less memory allocation #3882

merged 1 commit into from
Jun 4, 2023

Conversation

lasiar
Copy link
Contributor

@lasiar lasiar commented Jun 2, 2023

Use json encoder to write codeclimate report.

@boring-cyborg
Copy link

boring-cyborg bot commented Jun 2, 2023

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ldez ldez changed the title perf(codeclimate): less memory allocation. codeclimate: less memory allocation. Jun 3, 2023
@ldez
Copy link
Member

ldez commented Jun 3, 2023

Hello,

do you have a problem with the code climate reporter?
Can you explain your problem?

@ldez ldez self-requested a review June 3, 2023 01:54
@ldez ldez added waiting for: contributor feedback Requires additional feedback area: output Related to issue output labels Jun 3, 2023
@ldez ldez changed the title codeclimate: less memory allocation. codeclimate: less memory allocation Jun 3, 2023
@lasiar
Copy link
Contributor Author

lasiar commented Jun 4, 2023

@ldez

Hello,

do you have a problem with the code climate reporter? Can you explain your problem?

Hello, I don't have any problem, this PR just reduce allocation and memory usage.

Source code of benchmark.

result on my machine:

go test -run='^$' -bench=. -count=10

name old time/op new time/op delta
Climate-10 2.79µs ± 1% 2.56µs ± 1% -8.00% (p=0.000 n=9+8)
name old alloc/op new alloc/op delta
Climate-10 3.36kB ± 1% 2.23kB ± 3% -33.66% (p=0.000 n=10+9)
name old alloc/op new alloc/op delta
Climate-10 34.0 ± 0% 31.0 ± 0% -8.82% (p=0.000 n=10+10)

@ldez
Copy link
Member

ldez commented Jun 4, 2023 •
edited
Loading

Hello,

The problem with this kind of benchmark is in the real interest of this optimization.

The basic rule of optimization is to do it only when you have a performance problem.
There is a name for that: premature optimization.

In a general way, json.Marshal or json.Unmarshal produce more useful errors than json.NewDecoder and json.NewEncoder.

In the current situation, there's little chance of an error, and the change doesn't impact the maintainability, so we can accept this PR.
But I want to be clear that performance optimization should be led by performance problems and not micro-benchmarks.

@ldez ldez added enhancement New feature or improvement and removed waiting for: contributor feedback Requires additional feedback labels Jun 4, 2023
@lasiar @ldez
perf(codeclimate): less memory allocation.
637eba8 
Use json encoder to write codeclimate report.
@ldez ldez merged commit 8c519af into golangci:master Jun 4, 2023
@ldez ldez added this to the v1.53 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ldez ldez ldez approved these changes

Assignees

No one assigned

Labels

area: output Related to issue output enhancement New feature or improvement

Projects

None yet

Milestone

v1.53

Development

Successfully merging this pull request may close these issues.

3 participants

@lasiar @CLAassistant @ldez