Support multithreading in groupreduce #2491

nalimilan · 2020-10-17T21:43:29Z

Keep the default to a single thread until we find a reliable way of predicting a reasonably optimal number of threads.

nalimilan · 2020-10-17T21:56:31Z

src/groupeddataframe/splitapplycombine.jl

@@ -453,18 +453,22 @@ julia> combine(gd, :, AsTable(Not(:a)) => sum, renamecols=false)
 ```
 """
 function combine(f::Base.Callable, gd::GroupedDataFrame;
-                 keepkeys::Bool=true, ungroup::Bool=true, renamecols::Bool=true)
+                 keepkeys::Bool=true, ungroup::Bool=true, renamecols::Bool=true,
+                 nthreads::Int=NTHREADS)


Ideally we should use a keyword argument name that is standard across packages, but for now I haven't found examples of similar cases elsewhere.

We should also anticipate what it will look like once we switch the default to the automatic optimal number of threads. Would it be nthreads=nothing, nthreads=:auto, nthreads=0? We could also use another name: threads=true looks good at it also works with e.g. threads=5. Though using a different type forces recompilation, so maybe 0 is better as a default, even though it's not super logical.

bkamins · 2020-10-17T22:14:38Z

Note that after #2481 method signatures will change so this PR will have merge conflicts.

Keep the default to a single thread until we find a reliable way of predicting a reasonably optimal number of threads.

nalimilan · 2020-11-25T10:03:05Z

I've rebased against master. Should be good for a review now. I just hope CI really used 2 threads but it's not visible in the logs AFAICT. EDIT: seeing Codecov, probably not.

bkamins · 2020-11-25T10:10:05Z

EDIT: seeing Codecov, probably not.

@quinnj - could you please have a quick look at the CI set-up? Thank you!

src/groupeddataframe/fastaggregates.jl

nalimilan · 2020-11-25T10:26:44Z

Wait, I just did something completely stupid when setting the environment variable. I've committed a probable fix.

nalimilan · 2020-11-25T10:50:37Z

OK that worked!

nalimilan · 2020-11-25T21:47:11Z

I've added DataFrames.nthreads (called to choose the default nthreads) and DataFrames.nthreads! to set it, to make it easier to choose the number of threads once for a session, as requested by Jan.

tk3369 · 2020-11-25T22:14:42Z

I was just thinking about nthreads! sounding a little weird because it's not a verb. Given that it's a global setting, would it make sense to create a struct that holds on to all global settings? Then, the user can configure it via setproperty!. Just a thought...

nalimilan · 2020-11-26T09:25:36Z

I was just thinking about nthreads! sounding a little weird because it's not a verb. Given that it's a global setting, would it make sense to create a struct that holds on to all global settings? Then, the user can configure it via setproperty!. Just a thought...

Yes, I also thought about setnthreads!. That's an eternal question about naming conventions for setters in Julia. But note that we have Random.seed! already in the stdlib, which is quite similar.

Adding a struct for global settings sounds overkill at this point since it's not clear we're going to add more settings in the future. I'd rather keep this minimal, so that it can be extended if needed in the future, e.g. using Preferences.jl for storage across sessions.

bkamins · 2020-11-26T12:12:19Z

src/other/utils.jl

+Use [`DataFrames.nthreads`](@ref) to access the value.
+"""
+function nthreads!(n::Int)
+    n > 0 || throw(ArgumentError("n must be equal to or greater than 1 (got $n)"))


we might also cap this at Threads.nthreads() here so that later DataFrames.nthreads does report the number of threads that is going to be actually used

bkamins · 2020-11-26T12:16:03Z

I'd rather keep this minimal

I am OK with the current design as it is an implementation detail. Though probably having a mutable struct instead of Ref is more natural (but we can always change it in the future).

Regarding the naming - I am OK with nthreads!, but in general I do not have a super strong opinion here.

nalimilan · 2020-11-26T12:39:35Z

The Ref is an implementation detail, but we have to decide whether DataFrames.nthreads and DataFrames.nthreads! are part of the public API. If we're hesitant we could avoid mentioning these in the docs and only use them for benchmarking for now (like DataFrames.precompile).

bkamins · 2020-11-26T12:46:13Z

I am OK to have them as a part of the public API.

@StefanKarpinski - if Julia Base were to add an option to change number of available threads dynamically during the Julia session, would Threads.nthreads! would be a name for this operation?

bkamins · 2021-01-24T18:09:45Z

This can be closed given the other approach we took? Or we keep it open for the future?

nalimilan · 2021-03-06T12:05:15Z

This PR is complementary with the other threaded grouping PRs, since without it optimized reductions are not multithreaded. We should check whether we can identify conditions under which multithreading is faster than single threading.

bkamins · 2021-03-06T17:33:27Z

I have totally forgotten about this PR. I thought you have merged it :). So what is the status here - are you working on the condition or I should review it as is?

Thank you!

nalimilan · 2021-03-06T19:05:38Z

Given that we now automatically spawn as many tasks as there are threads in other places, requiring users to set NTHREADS for this function may be a bit weird. I guess we should try to automatically compute a reasonable number of threads -- but that turned out to be quite tricky.

bkamins · 2021-03-06T19:24:27Z

equiring users to set NTHREADS for this function may be a bit weird

I agree

but that turned out to be quite tricky.

Is it possible to give some safe condition though? (I do not remember the details of our earlier discussion)

nalimilan · 2021-03-08T09:07:37Z

Is it possible to give some safe condition though? (I do not remember the details of our earlier discussion)

Probably. I need to look at the data again. What's difficult is that there are several dimensions to cross: number of groups, number of rows, number of threads. And IIRC the overhead due to threading is larger when Julia was started with many threads.

Finally, there's always the possibility that all CPUs are already busy with another threaded operation, so that spawning multiple tasks is counter-productive. Maybe that's not a serious issue.

bkamins · 2021-03-08T09:28:48Z

Should I mark it as 1.0, or keep it 1.x and we will merge it when we are comfortable?

nalimilan · 2021-03-08T20:17:21Z

I think this doesn't have to happen before 1.0.

bkamins · 2021-03-15T09:43:08Z

src/groupeddataframe/fastaggregates.jl

+                start = 1 + ((tid - 1) * length(groups)) ÷ nt
+                stop = (tid * length(groups)) ÷ nt


avoid overflow on 32-bit machines, on 64-bit machine this is a no-op

Suggested change

start = 1 + ((tid - 1) * length(groups)) ÷ nt

stop = (tid * length(groups)) ÷ nt

start = 1 + ((tid - 1) * Int64(length(groups))) ÷ nt

stop = (tid * Int64(length(groups))) ÷ nt

bkamins · 2022-01-31T10:44:21Z

@nalimilan - what do we do with this PR? Close it (as it is likely outdated)?

In general - for the future we should rather add a kwarg allowing to choose if users wants to use multithreading c.f. #2992.

bkamins · 2022-02-20T12:37:09Z

bump

@nalimilan - what do you think we should do with this PR?

nalimilan · 2022-02-20T22:08:10Z

I'm not sure. There are two aspects to address:

If we decide to add keyword arguments to choose the number of threads (Allow functions in DataFrames.jl to pick how many threads they use #2992, Is possible to restrict number of threads? #2988), it would make sense to (rebase and) merge this.
Otherwise, we would need to find conditions under which using multiple threads is always faster. I haven't retried.

bkamins · 2022-02-21T09:46:43Z

@nalimilan - following our discussion I recommend to do what is written in #2988 (comment).

We would then implement this proposed change. There are the following things to be done to implement this change:

implement the feature of this PR
implement enabling/disabling threading of what you propose in this PR
implement enabling/disabling threading in general in _combine (across transformation operations)
implement ntasks keyword argument in all relevant functions

@nalimilan - can you please comment which parts of this list you would be willing to have a look at (I understand that 1, 2, and 4 must be covered in this PR, but 3 can be done in a separate follow-up PR that I can do if you prefer after this PR is finished but before 1.4 release)

Additional maintenance tasks:

add tests for all cases
create and run benchmarks
update docstrings
update documentation
update NEWS.md

nalimilan · 2022-02-23T08:07:08Z

OK I'll rebase this PR, but I'd rather leave point 3 for another one.

bkamins · 2022-02-23T10:04:48Z

Sure. Thank you!

bkamins · 2022-02-23T10:06:10Z

(probably starting a new branch and making proper changes + force push rather than rebase would be easier, as I am not sure by how much we have changed the sources you touched)

bkamins · 2022-06-12T18:32:15Z

@nalimilan - what are your plans with this PR 😄? (I am asking because it is getting so old that there is a risk that it is better to re-implement it from scratch if we want to have it given the recent changes)

nalimilan · 2022-06-14T07:39:25Z

Now that we have the threads keyword argument, we could allow passing an integer to enable multithreaded groupreduce. But it would be kind of weird that the default threads=true wouldn't enable multithreading here, while e.g. threads=2 would. Also threads=2 wouldn't actually limit the number of tasks to 2 as the existing code doesn't really allow doing that (nor should it). And anyway many people wouldn't benefit from it as they will probably expect threading to be automatic, just like it is in other cases.

So it would probably be worth experimenting more to find conditions under which we can be sure starting multiple tasks is faster.

bkamins · 2022-06-14T08:24:54Z

OK - so let us keep it open for now.

bkamins · 2022-12-02T10:08:00Z

I converted this PR to draft to avoid accidental merging.

nalimilan commented Oct 17, 2020

View reviewed changes

bkamins added grouping performance labels Oct 17, 2020

bkamins added this to the 1.x milestone Oct 17, 2020

nalimilan force-pushed the nl/threadedgrouping branch from b676e63 to bc4dc71 Compare October 17, 2020 22:25

nalimilan force-pushed the nl/threadedgrouping branch 2 times, most recently from 1a977d3 to 1933482 Compare November 24, 2020 22:48

Support multithreading in groupreduce

2d57734

Keep the default to a single thread until we find a reliable way of predicting a reasonably optimal number of threads.

nalimilan force-pushed the nl/threadedgrouping branch from 1933482 to 2d57734 Compare November 25, 2020 09:19

nalimilan marked this pull request as ready for review November 25, 2020 10:03

bkamins reviewed Nov 25, 2020

View reviewed changes

src/groupeddataframe/fastaggregates.jl Outdated Show resolved Hide resolved

Fix setting number of threads in CI

713d5b8

nalimilan added 4 commits November 25, 2020 21:53

Use Int

3b5addb

Disallow negative values for nthreads

ab76ff8

Add global default

3e225ad

Merge remote-tracking branch 'origin/master' into nl/threadedgrouping

011a9b8

nalimilan force-pushed the nl/threadedgrouping branch from ac55a99 to 011a9b8 Compare November 25, 2020 21:05

Fix

9595755

bkamins reviewed Nov 26, 2020

View reviewed changes

nalimilan mentioned this pull request Mar 15, 2021

Use multithreading in row_group_slots refarray method #2661

Merged

bkamins reviewed Mar 15, 2021

View reviewed changes

bkamins mentioned this pull request Feb 21, 2022

Is possible to restrict number of threads? #2988

Closed

nalimilan mentioned this pull request Apr 12, 2022

Do we want something like this? tkf/ParallelismHints.jl#1

Open

bkamins modified the milestones: 1.x, 1.5 Jun 19, 2022

bkamins mentioned this pull request Aug 24, 2022

Drop master branch? #3121

Closed

nalimilan changed the base branch from master to main August 25, 2022 19:34

bkamins marked this pull request as draft December 2, 2022 10:07

bkamins modified the milestones: 1.5, 1.x Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multithreading in groupreduce #2491

Support multithreading in groupreduce #2491

nalimilan commented Oct 17, 2020

nalimilan Oct 17, 2020

bkamins commented Oct 17, 2020

nalimilan commented Nov 25, 2020 •

edited

bkamins commented Nov 25, 2020

nalimilan commented Nov 25, 2020

nalimilan commented Nov 25, 2020

nalimilan commented Nov 25, 2020

tk3369 commented Nov 25, 2020

nalimilan commented Nov 26, 2020

bkamins Nov 26, 2020

bkamins commented Nov 26, 2020

nalimilan commented Nov 26, 2020

bkamins commented Nov 26, 2020

bkamins commented Jan 24, 2021

nalimilan commented Mar 6, 2021

bkamins commented Mar 6, 2021

nalimilan commented Mar 6, 2021

bkamins commented Mar 6, 2021

nalimilan commented Mar 8, 2021

bkamins commented Mar 8, 2021

nalimilan commented Mar 8, 2021

bkamins Mar 15, 2021

bkamins commented Jan 31, 2022

bkamins commented Feb 20, 2022

nalimilan commented Feb 20, 2022

bkamins commented Feb 21, 2022

nalimilan commented Feb 23, 2022

bkamins commented Feb 23, 2022

bkamins commented Feb 23, 2022

bkamins commented Jun 12, 2022

nalimilan commented Jun 14, 2022

bkamins commented Jun 14, 2022

bkamins commented Dec 2, 2022

		start = 1 + ((tid - 1) * length(groups)) ÷ nt
		stop = (tid * length(groups)) ÷ nt

Support multithreading in groupreduce #2491

Are you sure you want to change the base?

Support multithreading in groupreduce #2491

Conversation

nalimilan commented Oct 17, 2020

nalimilan Oct 17, 2020

Choose a reason for hiding this comment

bkamins commented Oct 17, 2020

nalimilan commented Nov 25, 2020 • edited

bkamins commented Nov 25, 2020

nalimilan commented Nov 25, 2020

nalimilan commented Nov 25, 2020

nalimilan commented Nov 25, 2020

tk3369 commented Nov 25, 2020

nalimilan commented Nov 26, 2020

bkamins Nov 26, 2020

Choose a reason for hiding this comment

bkamins commented Nov 26, 2020

nalimilan commented Nov 26, 2020

bkamins commented Nov 26, 2020

bkamins commented Jan 24, 2021

nalimilan commented Mar 6, 2021

bkamins commented Mar 6, 2021

nalimilan commented Mar 6, 2021

bkamins commented Mar 6, 2021

nalimilan commented Mar 8, 2021

bkamins commented Mar 8, 2021

nalimilan commented Mar 8, 2021

bkamins Mar 15, 2021

Choose a reason for hiding this comment

bkamins commented Jan 31, 2022

bkamins commented Feb 20, 2022

nalimilan commented Feb 20, 2022

bkamins commented Feb 21, 2022

nalimilan commented Feb 23, 2022

bkamins commented Feb 23, 2022

bkamins commented Feb 23, 2022

bkamins commented Jun 12, 2022

nalimilan commented Jun 14, 2022

bkamins commented Jun 14, 2022

bkamins commented Dec 2, 2022

nalimilan commented Nov 25, 2020 •

edited