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

Potential speedup from cyclocomp #1967

Closed
etiennebacher opened this issue May 17, 2023 · 8 comments
Closed

Potential speedup from cyclocomp #1967

etiennebacher opened this issue May 17, 2023 · 8 comments

Comments

@etiennebacher
Copy link

Hi, this is not really an issue with lintr but just to let you know that it could have a nice speedup (see example below) from a very simple PR in cyclocomp: MangoTheCat/cyclocomp#24

I know you're not responsible for cyclocomp but it looks like there was no activity there for almost 2 years, so if at some point you want to get your own fork of it, maybe you should include this PR.

That's it, feel free to close this issue and thanks for lintr of course ;)


With current cyclocomp:

text <- "x <- 1\n"

tmp <- tempfile()
cat(rep(text, 1000), file = tmp)

bench::mark(
  lintr::lint(tmp, lintr::cyclocomp_linter())
)
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 1 × 6
#>   expression                             min median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                           <bch> <bch:>     <dbl> <bch:byt>    <dbl>
#> 1 lintr::lint(tmp, lintr::cyclocomp_l… 9.52s  9.52s     0.105     319MB     18.4

With cyclocomp PR 24:

text <- "x <- 1\n"

tmp <- tempfile()
cat(rep(text, 1000), file = tmp)

bench::mark(
  lintr::lint(tmp, lintr::cyclocomp_linter())
)
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 1 × 6
#>   expression                             min median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                           <bch> <bch:>     <dbl> <bch:byt>    <dbl>
#> 1 lintr::lint(tmp, lintr::cyclocomp_l… 2.72s  2.72s     0.368     241MB     15.4
@IndrajeetPatil
Copy link
Collaborator

Thanks, @etiennebacher, for the suggestion!

Yeah, I also feel like it would be great if we either fork the repo to r-lib and incorporate a number of pending PRs or just import the functionality to {lintr} (MIT licence allows this). {cyclocomp} is stable enough that moving its source code to {lintr} shouldn't lead to additional maintenance woes.

WDYT, @AshesITR, @MichaelChirico?

@MichaelChirico
Copy link
Collaborator

a fork into r-lib makes sense to me...

I'm not sure who manages those, but I know xmlparsedata is a fork of the same author by @gaborcsardi, maybe he can offer some insight.

@etiennebacher
Copy link
Author

Small bump on this. I just tried on a real package that contains some large files (polars) and the timing for lint_package() went from 240 seconds to 174 seconds due to this simple PR in cyclocomp.

@MichaelChirico
Copy link
Collaborator

cc @hadley / @wch, what's the process for adopting a package like {cyclocomp} into the r-lib org?

I couldn't find any info about this poking around the org's GH page, nor does "r-lib.org" have its own page...

@hadley
Copy link
Member

hadley commented Aug 6, 2023

IMO the place to start is by emailing the maintainer of the package. If that works, we can transfer to cyclocomp r-lib and take over maintenance. The main r-lib restriction is that we (i.e. me + my team) have right of first refusal if the maintainer wants to stop maintaining, and similarly we'll guarantee maintenance (at least to the level of keeping on CRAN) for any package in r-lib.

@gaborcsardi
Copy link
Member

I am still the maintainer of cyclocomp on CRAN, but just wrote to the current maintainer of the GH package. But let's wait for the reply.

@gaborcsardi
Copy link
Member

OK, no answer, so I'll fork the cyclocomp repo into my account. CRAN also asked me to update the maintainer email, so there'll be a new release as well.

@etiennebacher
Copy link
Author

I see that cyclocomp was updated on CRAN and includes the PR mentioned at the beginning, so I'm closing this. Thanks @gaborcsardi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants