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

Performance improvements with if() + else() instead of ifelse() #1006

Merged
merged 8 commits into from
Sep 21, 2022
Merged

Performance improvements with if() + else() instead of ifelse() #1006

merged 8 commits into from
Sep 21, 2022

Conversation

IndrajeetPatil
Copy link
Collaborator

@codecov-commenter
Copy link

Codecov Report

Merging #1006 (6cb403c) into main (d80d061) will decrease coverage by 0.04%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
- Coverage   91.11%   91.07%   -0.05%     
==========================================
  Files          46       46              
  Lines        2635     2644       +9     
==========================================
+ Hits         2401     2408       +7     
- Misses        234      236       +2     
Impacted Files Coverage Δ
R/set-assert-args.R 90.47% <50.00%> (-2.39%) ⬇️
R/nested-to-tree.R 92.85% <66.66%> (-3.44%) ⬇️
R/detect-alignment-utils.R 97.18% <100.00%> (ø)
R/detect-alignment.R 97.80% <100.00%> (+0.07%) ⬆️
R/expr-is.R 86.11% <100.00%> (ø)
R/relevel.R 49.29% <100.00%> (+1.46%) ⬆️
R/rules-line-breaks.R 100.00% <100.00%> (ø)
R/style-guides.R 99.43% <100.00%> (ø)
R/token-create.R 96.22% <100.00%> (+0.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0d8b2ce is merged into main:

  •   :ballot_box_with_check:cache_applying: 28.4ms -> 28.3ms [-1.8%, +1.03%]
  •   :ballot_box_with_check:cache_recording: 1.3s -> 1.3s [-1.39%, +1.02%]
  •   :ballot_box_with_check:without_cache: 3.39s -> 3.4s [-1.13%, +2.09%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator Author

Unfortunately, not as impressive as I would have expected it to be.

So I will let you decide if this is worth merging.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0d8b2ce is merged into main:

  •   :rocket:cache_applying: 27.2ms -> 27ms [-1.42%, -0.17%]
  •   :ballot_box_with_check:cache_recording: 1.26s -> 1.26s [-0.68%, +0.13%]
  •   :ballot_box_with_check:without_cache: 3.33s -> 3.33s [-0.65%, +0.33%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0d8b2ce is merged into main:

  •   :ballot_box_with_check:cache_applying: 24.1ms -> 23.9ms [-1.41%, +0.21%]
  •   :ballot_box_with_check:cache_recording: 1.11s -> 1.11s [-0.63%, +1.24%]
  •   :ballot_box_with_check:without_cache: 2.9s -> 2.9s [-1.45%, +1.36%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator Author

The cache_applying benchmark does seem to improve - sometimes significant, and sometimes not - but it is always in the right direction here.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert left a comment

Choose a reason for hiding this comment

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

That benchmark is also the least of our concern because applying the cache is already 'instant'. I am ok with the changes. They certainly do not make things worse in terms of performance and readibility is not hurt too much. And the int conversions also make sense.

@IndrajeetPatil IndrajeetPatil merged commit 1f4437b into r-lib:main Sep 21, 2022
@IndrajeetPatil IndrajeetPatil changed the title Check for performance improvements with if() + else() instead of ifelse() Performance improvements with if() + else() instead of ifelse() Sep 21, 2022
@IndrajeetPatil IndrajeetPatil deleted the perf_ifelse branch September 21, 2022 06:34
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.

3 participants