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

Jump to bottom

mutate() performance improvements #4858

Merged
merged 26 commits into from Feb 24, 2020
Merged

mutate() performance improvements #4858

merged 26 commits into from Feb 24, 2020

Conversation

romainfrancois
Copy link
Member

No description provided.

@romainfrancois
Copy link
Member Author

The first change is to make new_grouped_df() slightly faster by updating tibble::new_tibble() to more carefully deal with attributes in tidyverse/tibble#717

image

->

image

@romainfrancois
Copy link
Member Author

skipping o_rows <- vec_order(vec_c(!!!rows, .ptype = integer())) and using the new internal vec_sprinkle() instead of vec_slice(vec_c(!!!chunks, .ptype = ptype), o_rows) gives a nice boost when organizing chunks into the right positions:

image

@romainfrancois
Copy link
Member Author

This is starting to be reasonable, with both changes from tidyverse/tibble#717

image

DavisVaughan
DavisVaughan reviewed Feb 14, 2020
View reviewed changes
src/mutate.cpp Outdated Show resolved Hide resolved
lionel-
lionel- reviewed Feb 17, 2020
View reviewed changes
src/mutate.cpp Outdated Show resolved Hide resolved
src/mutate.cpp Outdated Show resolved Hide resolved
src/mutate.cpp Outdated Show resolved Hide resolved
src/mutate.cpp Outdated Show resolved Hide resolved
src/mutate.cpp Outdated Show resolved Hide resolved
R/mutate.R Outdated Show resolved Hide resolved
@romainfrancois
Copy link
Member Author

I think I've incorporated suggestions from both @lionel- and @DavisVaughan now. My next step is to pursue re-injecting hybrid evaluation, but this is something that I'd like to do in a different branch.

@romainfrancois romainfrancois marked this pull request as ready for review February 18, 2020 09:03
@lionel- lionel- mentioned this pull request Feb 18, 2020
Closed
@romainfrancois romainfrancois mentioned this pull request Feb 20, 2020
Merged
romainfrancois and others added 19 commits February 21, 2020 14:11
@romainfrancois
🧞 ♂️ tidyverse/tibble#717
91acb09
@romainfrancois
prototype vec_sprinkle()
837208d
@romainfrancois
require fallback for more types
34df70a
@romainfrancois
handle names when sprinkling
c7b3a45
@romainfrancois
need to vec_cast() all the chunks to common type
7f7fcac
@romainfrancois
make all sprinkle steps internal with short_vec_init() and vec_cast()…
a956f89 
… callables
@romainfrancois
internal recycling by repeatedly calling vec_assign_impl() with the s…
6ca65c1 
…prinkling
@romainfrancois
rows_i_j needs PROTECT()
a23e93e
@romainfrancois
swing back to master tibble
5b91850
@romainfrancois
globalVariables(o_rows) because it is defined lazily
04bc6e8
@romainfrancois
rename to vec_unchop() and move arguments around
0eb2608
@romainfrancois @lionel-
Apply suggestions from code review
01ec7af 
Co-Authored-By: Lionel Henry <lionel.hry@gmail.com>
@romainfrancois @lionel-
Apply suggestions from code review
88b932e 
Co-Authored-By: Lionel Henry <lionel.hry@gmail.com>
@romainfrancois
PROTECT(Rf_getAttrib())
ecc1f46
@romainfrancois
skip again for a different reason
773e777
@romainfrancois
intercept mutate() recycling problems earlier
141b6f7
@romainfrancois
using vctrs::vec_unchop()
04c3acf
@romainfrancois
using the callable vctrs::short_vec_recycle() so that chunks are recy…
c777709 
…cled to fit
@romainfrancois
No need to hang on to rows_lengths
83db132
@romainfrancois
Copy link
Member Author

Now that r-lib/vctrs#834 is merged, perhaps we can merge this

@romainfrancois romainfrancois merged commit e5ca96d into master Feb 24, 2020
@romainfrancois romainfrancois deleted the perf_mutate branch February 24, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lionel- lionel- lionel- left review comments

@DavisVaughan DavisVaughan DavisVaughan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@romainfrancois @lionel- @DavisVaughan