KEMBAR78
Remove unnecessary allocations and clones by vbrandl · Pull Request #350 · dotenv-linter/dotenv-linter · GitHub
Skip to content

Remove unnecessary allocations and clones #350

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 13 commits into from
Dec 8, 2020
Merged

Remove unnecessary allocations and clones #350

merged 13 commits into from
Dec 8, 2020

Conversation

@vbrandl
Copy link
Contributor

@vbrandl vbrandl commented Dec 5, 2020 •
edited
Loading

Another PR by me :)

When working on my last change, I saw a few unnecessary clones, to_strings, to_owneds and so on. This change tries to remove them where possible and therefore improve the runtime performance by getting rid of a few heap allocations and memory copies.

There are no new features, so I didn't write new tests, but the existing tests still pass :)

✔ Checklist:

  • Commit messages have been written in Conventional Commits format;
  • This PR has been added to CHANGELOG.md (at the top of the list);
  • Tests for the changes have been added (for bug fixes / features);
  • Docs have been added / updated on the dotenv-linter.github.io (for bug fixes / features).

Valentin Brandl added 4 commits December 5, 2020 15:02
@vbrandl vbrandl changed the title Remove unnecessary allocations and clonse Remove unnecessary allocations and clones Dec 5, 2020
@codecov-io
Copy link

codecov-io commented Dec 5, 2020 •
edited
Loading

Codecov Report

Merging #350 (29e5c92) into master (110b236) will decrease coverage by 1.04%.
The diff coverage is 82.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
- Coverage   96.11%   95.06%   -1.05%     
==========================================
  Files          34       34              
  Lines        2419     2310     -109     
==========================================
- Hits         2325     2196     -129     
- Misses         94      114      +20
Impacted Files Coverage Δ
src/checks/duplicated_key.rs 100.00% <ø> (ø)
src/checks/ending_blank_line.rs 81.81% <0.00%> (-9.36%) ⬇️
src/checks/incorrect_delimiter.rs 95.45% <ø> (-0.25%) ⬇️
src/checks/key_without_value.rs 92.30% <ø> (-0.20%) ⬇️
src/checks/lowercase_key.rs 92.30% <ø> (-0.38%) ⬇️
src/checks/quote_character.rs 88.67% <0.00%> (-5.97%) ⬇️
src/checks/unordered_key.rs 100.00% <ø> (ø)
src/fixes.rs 92.41% <ø> (-0.49%) ⬇️
src/fixes/key_without_value.rs 100.00% <ø> (ø)
src/fixes/leading_character.rs 98.80% <ø> (-0.06%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 110b236...29e5c92. Read the comment docs.

@vbrandl
Copy link
Contributor Author

vbrandl commented Dec 5, 2020

I'm not familiar with codecov. Is there a way to see, which branch was covered by testing before, and is not covered after this change?

@vbrandl
Copy link
Contributor Author

vbrandl commented Dec 5, 2020

1a5e70a is a personal preference. I like the iterator API better than for loops. If you are unhappy with any of the changes, I'll revert those.

mgrachev
mgrachev reviewed Dec 8, 2020
View reviewed changes
@mgrachev
Copy link
Member

mgrachev commented Dec 8, 2020

1a5e70a is a personal preference. I like the iterator API better than for loops. If you are unhappy with any of the changes, I'll revert those.

I like this approach too 🙂

@mgrachev
Copy link
Member

mgrachev commented Dec 8, 2020 •
edited
Loading

I'm not familiar with codecov. Is there a way to see, which branch was covered by testing before, and is not covered after this change?

You can find the diff here https://app.codecov.io/gh/dotenv-linter/dotenv-linter/compare/350/diff

@mgrachev
Copy link
Member

mgrachev commented Dec 8, 2020

@vbrandl Thank you for your contribution! 🔥

  • This PR has been added to CHANGELOG.md (at the top of the list);

Please do it 🙂

@vbrandl
Copy link
Contributor Author

vbrandl commented Dec 8, 2020

I'm not familiar with codecov. Is there a way to see, which branch was covered by testing before, and is not covered after this change?

You can find the diff here https://app.codecov.io/gh/dotenv-linter/dotenv-linter/compare/350/diff

I still cannot really tell, which branches, that were covered before, are not covered now, so I know for which cases, new tests are required.

mgrachev
mgrachev reviewed Dec 8, 2020
View reviewed changes
@mgrachev mgrachev merged commit dcc4afc into dotenv-linter:master Dec 8, 2020
@vbrandl
Copy link
Contributor Author

vbrandl commented Dec 8, 2020

It might actually be interesting to rerun the benchmarks from #351 to see, if these changes have a measurable impact

@vbrandl vbrandl deleted the fix/unnecessary-allocations branch December 8, 2020 17:36
@mgrachev
Copy link
Member

mgrachev commented Dec 8, 2020

@vbrandl Thanks a lot 🚀 You did a great job 👍

@mgrachev
Copy link
Member

mgrachev commented Dec 8, 2020 •
edited
Loading

It might actually be interesting to rerun the benchmarks from #351 to see, if these changes have a measurable impact

Command Mean [ms] Min [ms] Max [ms] Relative
dotenv-linter/dotenv-linter .env 2.7 ± 0.4 2.0 4.3 1.00
wemake-services/dotenv-linter .env 162.6 ± 12.1 153.0 201.3 60.83 ± 10.20

@vbrandl
Copy link
Contributor Author

vbrandl commented Dec 8, 2020

I count cutting down the max execution time by almost 50% as a win, even if it's indistinguishable for humans :D

@mgrachev
Copy link
Member

mgrachev commented Dec 8, 2020

I count cutting down the max execution time by almost 50% as a win, even if it's indistinguishable for humans :D

You are awesome ⚡️

@mgrachev mgrachev mentioned this pull request Dec 8, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgrachev mgrachev mgrachev approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vbrandl @codecov-io @mgrachev