This repository has been archived by the owner on May 5, 2019. It is now read-only.
Specialize row_group_slots() and findrow() on column types to improve performance #79
Conversation
This file contains 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
nalimilan
commented
Jul 11, 2017
src/datatablerow/datatablerow.jl
Outdated
| isequal_row(cols::Tuple{AbstractVector}, r1::Int, r2::Int) = | ||
| isequal_colel(cols[1][r1], cols[1][r2]) | ||
| isequal_row(cols::Tuple{Vararg{AbstractVector}}, r1::Int, r2::Int) = | ||
| isequal_colel(cols[1][r1], cols[1][r2]) && isequal_row(Base.tail(cols), r1, r2) |
There was a problem hiding this comment.
This recursive trick allows the compiler to generate well-typed code even when the tuples have heterogeneous eltypes.
|
Looks like I spoke too soon about EDIT: also, using the benchmark from this Discourse post, performance is similar to DataFrames and only slightly worse than Pandas, when it was previously terrible. |
|
The by performance is pretty close to Pandas with the Discourse example. On joins, Pandas is 2x faster and R's data.table is 3-4x faster. |
… performance Looping over columns is very slow when their type is unknown at compile time. Specialize the method on the types of the key (grouping) columns by passing a tuple of columns rather than a DataTable. This will force compiling a specific method for each combination of key types, but their number should remain relatively low and the one-time cost is worth it. This dramatically improves performance of groupby(), but does not have a large effect on join() since it is very inefficient in other areas. Also add return type assertion for rowhash(). The fact that the type of the columns isn't known at compile time appears to confuse inference, which isn't able to detect that this function always returns UInt. This reduces a lot the number of allocations when calling join(), but doesn't really change performance.
4 tasks
This was referenced Oct 9, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Looping over columns is very slow when their type is unknown at compile time.
Specialize the method on the types of the key (grouping) columns by passing
a tuple of columns rather than a DataTable. This will force compiling a specific
method for each combination of key types, but their number should remain relatively low
and the one-time cost is worth it.
This dramatically improves performance of groupby(), but does not have a large
effect on join() since it is very inefficient in other areas.
This is an alternative to #76. We should probably merge it anyway, even if we merge #76 later, as it improves the performance of the current algorithm, making it possible to compare the merits of both approaches.
In the simple test used in #76, this makes grouping faster than DataFrames, and even slightly faster than #76. More benchmarks would be needed to check whether this is also the case for other scenarios. Unfortunately,
joinremains much slower than DataFrames for unrelated reasons.The downside of this change is that the method will be recompiled for each combination of column types used for grouping. I think that's OK since the number of grouping columns is generally low, and the number of different types is limited too. Though I should note the approach in #76 only compiles one method for each column types, not for each of their combinations.
Cc: @cjprybol @alyst