Adjust the max load factor of the dictionary to 50%#1760
Merged
Conversation
DavisVaughan
marked this pull request as ready for review
December 22, 2022 16:51
lionel-
approved these changes
Dec 23, 2022
lionel-
left a comment
Member
There was a problem hiding this comment.
Interesting analysis! It seems reasonable to favour speed over space.
| } | ||
|
|
||
| const double load_adjusted_size = x_size / 0.77; | ||
| const double load_adjusted_size = x_size / 0.50; |
Member
There was a problem hiding this comment.
Maybe fetch the load factor from an envvar at load time for easier experimentation?
Member
There was a problem hiding this comment.
oh I guess a "vctrs:::" global option makes more sense
Member
Author
There was a problem hiding this comment.
I think it is reasonable to do this if it comes up again. But for now I don't feel the need to add in the little bit of extra complexity for something that I am hopeful won't need to change again
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've been studying our dictionary implementation to see if I can improve it in any way, since it is used in so many functions.
One thing that stood out was that our max load factor is 77%. I think this is quite high, actually. Base R uses 50%, FWIW https://github.com/wch/r-source/blob/e3f31ad3039eaade6bf9bb0d9201eb13ea33cf66/src/main/unique.c#L439.
I found a few images that show that as the load factor gets close to ~70%, the number of collisions sharply increases. The medium article suggests that quadratic probing (which we use) has a surprisingly bad result for "average time to figure out if the table contains x" as the load factor increases above 50%.
https://cs.stackexchange.com/questions/10273/hash-table-collision-probability
https://medium.com/geekculture/performance-of-hash-implementations-on-various-workloads-fedac579a39b#f31c
With all this in mind, I did some experiments with a 50% load factor and was pretty happy with what I saw. I suggest that we move to either a 50% or 60% load factor unconditionally. I think anything less than ~65% is probably going to result in significant improvement, so I'm ok if we don't want to move all the way to 50%, but I don't see much downside in doing so.
I'll show a number of examples below. I temporarily added a little helper that lets me dynamically switch the load factor with
with_load_50()andwith_load_77()to easily show the differences. You can see it in the commits.This is particularly meaningful for functions that look up values in one vector using a dictionary built on another vector (like
vec_match/in()andvec_set_intersect()). The dictionary is built onhaystackforvec_match(), and it is completely reasonable for each element ofhaystackto be unique, so as the size of thehaystackapproaches the 77% load factor size, that means there is only 23% of the table left for "new" values inneedlesto get mapped to, which results in a lot of collisions as values inneedlesare first mapped to an existinghaystackvalue, compared against it to find it is not the same value, and then quadratic probing generates a new probe, and this continues until we hit one of those empty slots in the 23% of the table that is still free.In this first example, I'll use a
haystackof size 1,600,000 with all unique values. With our current approach, this generates:If you do the same computations with a 50% max load factor, you get:
The
needlessample from 10,000,000 values, so a much wider range thanhaystack, meaning there will be a lot of unmatched values.This also extends out to data frames. For data frames the equality check is more expensive, so if we can reduce the number of collisions that directly reduces the number of equality checks, which is a double win! This data frame example again shows the "worst" case of all unique values + close to 77% load factor, and then it also shows a slightly less bad case where the haystack can have duplicates, lowering the current approach's load factor to around 63%, but the new approach still does better.
Now lets take a look at an example where in theory our changes shouldn't help much at all.
With 9,500,000 unique
haystackvalues, we'd get:So the load factor would hit 56% in the current approach, which really shouldn't hurt performance much. And we indeed do see that the timings aren't very different here. The 50% load does slightly better, at the cost of more memory because the next power of 2 is much larger so we need a larger dictionary.
This is typically less useful with functions like
vec_unique_loc(), because those only build the dictionary, they don't also use a filled dictionary (which might be near that 77% max capacity) to look up values of some other vector in it, which is typically the painful part.But it can still be useful when
xhas a lot of unique values in it, as just building the dictionary will slow down as we approach that 77% load factor.It also seems like it doesn't hurt very much in cases where we don't have many unique values (i.e. we don't really approach the max load factor). It does seem to be a little slower.
This also helps with the set operations, like
vec_set_intersect(), which builds the dictionary onxand looksyup in it. Whenxhas a lot of unique values (which is definitely plausible, considering it is supposed to be a set which doesn't have duplicates), the lower max load factor makes a big difference if the size ofxforces us to approach the current 77% max load factor.And it seems like spending a little bit more memory when we don't actually need it (few uniques) doesn't hurt much.