Skip to content

Avoid paste0 overhead in is_lint_level #2360

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

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Avoid paste0 overhead in is_lint_level #2360

merged 3 commits into from
Nov 28, 2023

Conversation

MichaelChirico
Copy link
Collaborator

Closes #2358

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f6453b6) 99.13% compared to head (649b9f9) 99.13%.

❗ Current head 649b9f9 differs from pull request most recent head e61c5b0. Consider uploading reports for the commit e61c5b0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2360      +/-   ##
==========================================
- Coverage   99.13%   99.13%   -0.01%     
==========================================
  Files         124      124              
  Lines        5574     5573       -1     
==========================================
- Hits         5526     5525       -1     
  Misses         48       48              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Can you benchmark switch() vs. if() first?

@IndrajeetPatil
Copy link
Collaborator

I had done a quick benchmarking locally that I am producing here, but not sure if you are actually looking for benchmark for lint_package() (e.g.) after this change. I didn't do the latter before approving.

level <- "expression"

bench::mark(
  "if" = paste0(if (level == "file") "full_", "parsed_content"),
  "switch" = switch(level,
    file = "full_parsed_content",
    expression = "parsed_content"
  ),
  iterations = 1000,
  time_unit = "us"
)
#> # A tibble: 2 × 6
#>   expression   min median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <dbl>  <dbl>     <dbl> <bch:byt>    <dbl>
#> 1 if         0.902 0.984    881984.        0B        0
#> 2 switch     0     0.0410 21351784.        0B        0

Created on 2023-11-28 with reprex v2.0.2

@AshesITR
Copy link
Collaborator

I was thinking if (level == "expression") "parsed_content" else "full_parsed_content", still without paste0().

@IndrajeetPatil
Copy link
Collaborator

Ah, I see. Here it is:

level <- "expression"

bench::mark(
  "if" = if (level == "file")  "full_parsed_content" else "parsed_content",
  "switch" = switch(level,
                    file = "full_parsed_content",
                    expression = "parsed_content"
  ),
  iterations = 1000,
  time_unit = "us"
)
#> # A tibble: 2 × 6
#>   expression    min median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>  <dbl>  <dbl>     <dbl> <bch:byt>    <dbl>
#> 1 if         0.0410 0.0820  6850171.        0B        0
#> 2 switch     0      0.0410 19969240.        0B        0

Created on 2023-11-28 with reprex v2.0.2

@AshesITR
Copy link
Collaborator

On a computer now, could check,'

level <- "expression"

bench::mark(
  "if" = if (level == "expression") "parsed_content" else "full_parsed_content",
  "switch" = switch(level,
    file = "full_parsed_content",
    expression = "parsed_content"
  )
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 if            120ns    128ns  6010347.        0B        0
#> 2 switch         76ns     83ns  9321876.        0B        0

level <- "file"

bench::mark(
  "if" = if (level == "expression") "parsed_content" else "full_parsed_content",
  "switch" = switch(level,
    file = "full_parsed_content",
    expression = "parsed_content"
  )
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 if          123.1ns  132.1ns  6368058.        0B        0
#> 2 switch       68.9ns   75.1ns 10365642.        0B        0

Created on 2023-11-28 with reprex v2.0.2

@AshesITR
Copy link
Collaborator

Here are some larger benchmarks:

expression

# A tibble: 2 × 13
  expression      min   median `itr/sec` mem_alloc `gc/sec`   n_itr  n_gc total_time result    memory             time                    gc                       
  <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>   <int> <dbl>   <bch:tm> <list>    <list>             <list>                  <list>                   
1 if            115ns    129ns  6658306.        0B     17.3 9999974    26       1.5s <chr [1]> <Rprofmem [0 × 3]> <bench_tm [10,000,000]> <tibble [10,000,000 × 3]>
2 switch       70.9ns     80ns 10382848.        0B     26.0 9999975    25    963.1ms <chr [1]> <Rprofmem [0 × 3]> <bench_tm [10,000,000]> <tibble [10,000,000 × 3]>

file

# A tibble: 2 × 13
  expression      min   median `itr/sec` mem_alloc `gc/sec`   n_itr  n_gc total_time result    memory             time                    gc                       
  <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>   <int> <dbl>   <bch:tm> <list>    <list>             <list>                  <list>                   
1 if            118ns  134.1ns  6591956.        0B     16.5 9999975    25      1.52s <chr [1]> <Rprofmem [0 × 3]> <bench_tm [10,000,000]> <tibble [10,000,000 × 3]>
2 switch         63ns   70.1ns 11524237.        0B     28.8 9999975    25   867.73ms <chr [1]> <Rprofmem [0 × 3]> <bench_tm [10,000,000]> <tibble [10,000,000 × 3]>

@AshesITR AshesITR merged commit 6d2b327 into main Nov 28, 2023
@AshesITR AshesITR deleted the paste0 branch November 28, 2023 07:52
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.

Speed up is_lint_level() by avoiding a paste0() call.
4 participants