Use refpool optimized method for integer grouping

[nalimilan]

main:

julia> using DataFrames, BenchmarkTools

julia> df = DataFrame(x=rand(1:10, 100_000), y=rand(10:20, 100_000));

julia> @btime groupby(df, :x);
  730.118 μs (31 allocations: 2.53 MiB)

julia> @btime groupby(df, [:x, :y]);
  1.091 ms (38 allocations: 2.53 MiB)

julia> df = DataFrame(x=rand(1:1000, 100_000), y=rand(1000:2000, 100_000));

julia> @btime groupby(df, :x);
  1.009 ms (31 allocations: 2.53 MiB)

julia> @btime groupby(df, [:x, :y]);
  2.690 ms (38 allocations: 2.53 MiB)

PR (updated to latest commits):

jjulia> using DataFrames, BenchmarkTools

julia> df = DataFrame(x=rand(1:10, 100_000), y=rand(10:20, 100_000));

julia> @btime groupby(df, :x);
  186.219 μs (58 allocations: 784.46 KiB)

julia> @btime groupby(df, [:x, :y]);
  296.452 μs (85 allocations: 785.84 KiB)

julia> df = DataFrame(x=rand(1:1000, 100_000), y=rand(1000:2000, 100_000));

julia> @btime groupby(df, :x);
  187.802 μs (58 allocations: 785.43 KiB)

# Too many combinations, falling back to generic method
julia> @btime groupby(df, [:x, :y]);
  2.982 ms (98 allocations: 2.53 MiB)

[nalimilan]

It seems that even above this threshold the refpool approach is faster. But I'm not sure how much memory we're willing to allocate for speed.

[nalimilan]

Last commit will fail until JuliaData/Missings.jl#126.

[bkamins]

Looks very nice. We only need to make sure we correctly disable trying this path in corner cases (like BigInt vector that has very large values, but with small range) as otherwise we will have overflow.

[bkamins]

Small suggestions:

1. check that min is less of equal than max
2. then no need to check typemin(Int)
3. also +2 is needed as we potentially have Missing to handle

Suggested change

	@assert max < typemax(Int) - 1
	@assert typemin(Int) <= widen(max) - widen(min) + 1 < typemax(Int)
	@assert min <= max < typemax(Int) - 1
	@assert widen(max) - widen(min) + 2 < typemax(Int)

[nalimilan]

I had to refactor this part but I've tried to reuse your suggestion to simplify checks. Let me know what you think.

[bkamins]

as above you have to use +2 to allow to store missing

Suggested change

	widen(maxval) - widen(minval) + 1 <= 2 * length(x) < typemax(Int)
	widen(maxval) - widen(minval) + 2 <= 2 * length(x) < typemax(Int)

[nalimilan]

Indeed. I've changed the code to store this in an ngroups variable to make things clearer.

[bkamins]

I would also add a test that on 32-bit machine uses slow path and on 64-bit machine uses fast path (as Int is defined differently on both) and another test that on 64-bit machine uses slow path because the values are too large.

[nalimilan]

I've added checks and tests for overflows. I've also bumped into an already existing bug with skipmissing=true when there are only missing values so I've added more tests for that.

[bkamins]

Have you seen my comments I left today in the morning?

[nalimilan]

I would also add a test that on 32-bit machine uses slow path and on 64-bit machine uses fast path (as Int is defined differently on both) and another test that on 64-bit machine uses slow path because the values are too large.

I've added tests that the fast path is used just below typemax(Int), and correctness tests just above it.

[bkamins]

are we sure v - x.offset will not overflow here?

[bkamins]

OK - I guess this line answers my question above. If we initialize offset to what you propose then we are safe. Maybe just add a comment above what offset means for IntegerRefarray type?

[nalimilan]

I've added a comment.

[bkamins]

it is interesting here that 2*length(x) can overflow if we are on 32 bit machine, but then it will be negative so the condition is correct (simply we will not use a fast path then). But if you want to be correct maybe better write Int64(2) * length(x)?

[nalimilan]

Woops, that's indeed not super clean. I've switched to Int64 (including in two similar occurrences).

[bkamins]

Have you checked that using | here is is optimized by the compiler correctly? What I mean is that when eltype(x) <: Integer is this check a no-op?

[nalimilan]

Yes, that's a good point, but I had tested it and it works (which is really nice).

[bkamins]

Looks good. Thank you. Can you just please make sure that in the points I have just raised we are clear what happens?

[bkamins]

Thank you. Looks good now. These overflow things are tricky.