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

Remove bottlenecks for styler #348

Merged
merged 9 commits into from Jan 15, 2018
Merged

Remove bottlenecks for styler #348

merged 9 commits into from Jan 15, 2018

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Dec 30, 2017

Now running ~3x faster on the tidyr package (60s -> 20s), CC @lorenzwalthert.

@codecov
Copy link

codecov bot commented Dec 30, 2017

Codecov Report

Merging #348 into master will increase coverage by 4.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   87.43%   91.61%   +4.18%     
==========================================
  Files          24       23       -1     
  Lines        1122     1050      -72     
==========================================
- Hits          981      962      -19     
+ Misses        141       88      -53
Impacted Files Coverage Δ
R/as_tibble.R 100% <100%> (ø) ⬆️
R/new.R 100% <100%> (ø) ⬆️
R/tibble.R 90.38% <100%> (-7.77%) ⬇️
R/tbl-df.r 98.48% <100%> (-1.52%) ⬇️
R/add.R 100% <100%> (ø) ⬆️
R/tribble.R 100% <100%> (ø) ⬆️
R/enframe.R 100% <100%> (ø) ⬆️

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 dd08491...bd6af96. Read the comment docs.

@lorenzwalthert
Copy link
Contributor

Ok cool. Need to check how much performance styler gains through these optimizations.

R/tbl-df.r Outdated
if (!exact) {
warningc("exact ignored")
}
if (missing(j)) {
return(unclass(x)[[i]])
Copy link
Member Author

Choose a reason for hiding this comment

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

@lionel-: Is there a helper for unclass(x))[[i]] in rlang?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet, maybe you can use .subset2()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, thanks!

@lorenzwalthert
Copy link
Contributor

I just ran devtools::test() on styler with this patch and it takes only ~54% of the time compared to the current CRAN version of tibble! Pretty amazing.

@lionel-
Copy link
Member

lionel- commented Jan 3, 2018

I'm thinking that the purrr compats should use purrr's C implementation to avoid the slowdown. It doesn't cost a lot to compile a little embedded C file.

@krlmlr
Copy link
Member Author

krlmlr commented Jan 3, 2018

Maybe move the elementary purrr functions to rlang, CC @hadley?

I've seen some slowdown due to rlang's very careful input checking. I'm really looking forward to the C implementation of the is_() family of functions ;-)

@lionel-
Copy link
Member

lionel- commented Jan 3, 2018

sorry, it's planned but I haven't had time to do it yet :(

Expression capture and interpolation got a lot faster though, and I'll finish porting eval_tidy() to C for the next release as well.

@lionel-
Copy link
Member

lionel- commented Jan 3, 2018

Most of rlang will be implemented in C eventually.

@krlmlr
Copy link
Member Author

krlmlr commented Jan 3, 2018

Thanks for the heads up, I really appreciate your work on rlang!

@krlmlr krlmlr merged commit ec6a5cc into master Jan 15, 2018
@krlmlr krlmlr deleted the f-fast branch January 15, 2018 11:13
@lorenzwalthert
Copy link
Contributor

Cool 😊

@lorenzwalthert
Copy link
Contributor

Should we require a minimal version of tibble for styler to make sure the speed improvements take effect? Probably not, right?

@hadley
Copy link
Member

hadley commented Feb 10, 2018

I think it's reasonable to require it

@krlmlr
Copy link
Member Author

krlmlr commented Feb 10, 2018

I'm using update.packages(ask = FALSE); usethis::use_tidy_versions() to achieve this, see r-lib/usethis#179.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants