Make task splitting in TTreeProcessorMT more sensible with many input files. by bendavid · Pull Request #7106 · root-project/root

bendavid · 2021-01-27T17:18:36Z

Previously if running with a large number of input files (few hundred or even a thousand+ could be typical for large datasets processed into flat trees with grid jobs) one could end up with a huge number of tasks (at minimum nThreads * nFiles).

This makes the splitting aware of the number of files, taken into account for both the default splitting and the function used for user configuration, which also changes name.

phsft-bot · 2021-01-27T17:18:39Z

Can one of the admins verify this patch?

bendavid · 2021-01-27T17:19:32Z

Let me know what the preferred way would be to add a deprecation warning if a user calls SetMaxTasksPerFilePerWorker, since they should now call SetTasksPerWorker instead.

oshadura · 2021-01-29T11:03:18Z

@phsft-bot build!

phsft-bot · 2021-01-29T11:03:24Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-01-29T11:32:10Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-01-29T11:41:22Z

Build failed on mac1014/python3.
Running on macphsft17.dyndns.cern.ch:/build/jenkins/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-01-29T11:55:31Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on null:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-01-29T12:06:10Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-01-29T12:13:45Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-01-29T13:28:11Z

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

eguiraud · 2021-01-29T14:00:16Z

Hi @bendavid , thanks a lot, I will take a look asap (I see that the test failures are desired -- I can take care of updating the corresponding roottest test).

phsft-bot · 2021-01-29T15:03:52Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

eguiraud

Thanks Josh, looks good to me, and I can take care of deprecating fgMaxTasksPerFilePerWorker & friends. I don't 100% trust my Friday evening brain though, so I'll give it a second thought on Monday and merge then 😄

tree/treeplayer/src/TTreeProcessorMT.cxx

eguiraud · 2021-02-01T12:48:44Z

@bendavid do you have anything against renaming {S,G}etTaksPerWorker to {S,G}etMaxTasksPerWorker? So it's clear that it's not a precise setting.

bendavid · 2021-02-01T12:51:01Z

No strong feelings, but the purpose of the name change was to reflect that it's not a hard upper ceiling either, just a target.

eguiraud · 2021-02-01T13:07:45Z

Let me see if I understand correctly: with these changes every file has a hard cap at ceil(tasksPerWorker * nWorkers / nFiles) tasks, so in total we'll have at most ceil(tasksPerWorker * nWorkers / nFiles) * nFiles tasks. So tasksPerWorker*nWorkers is a hard upper ceiling to the total number of tasks, modulo a potential (EDIT: maximum of) nFiles tasks more due to the effect of ceil. Was that what you were referring to, or am I missing something else?

bendavid · 2021-02-01T13:12:28Z

Yes that's right.

eguiraud · 2021-02-01T13:22:17Z

Alright. Indeed the extreme case in which a user sets fgTasksPerWorker to 1 but anyway ends up with one task per file might be surprising if we call it SetMaxTasksPerWorker (EDIT: but also if we call it SetTasksPerWorker). 🤔 SetTasksPerWorkerHint? That should manage user expectations, and I can add a docstring explaining that we will do our best to honor that given internal constraints. It's verbose, but it's an expert interface, I'd rather be verbose than opaque.

eguiraud · 2021-02-08T13:57:50Z

@bendavid can you please switch the name to SetTasksPerWorkerHint and do a force push? Then I'll add the deprecation of the old method to this same PR -- I guess I will make SetMaxTasksPerFilePerWorker(n) print a deprecation warning and in the meanwhile I'll make it equivalent to SetTasksPerWorkerHint(n*nFiles) (not quite the same as before, but better than a no-op I guess).

bendavid · 2021-02-16T18:57:16Z

Done.

eguiraud · 2021-02-17T18:01:53Z

@phsft-bot build please

phsft-bot · 2021-02-17T18:01:59Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-02-17T18:24:52Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on null:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-17T18:25:26Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-17T18:36:14Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-17T19:08:19Z

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-17T20:02:55Z

Build failed on mac1014/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-17T20:20:54Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-17T22:47:15Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-02-17T22:47:14.892Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1070 (ctest_start):

tree/treeplayer/src/TTreeProcessorMT.cxx

Co-authored-by: Enric Tejedor Saavedra <[email protected]>

eguiraud · 2021-02-19T13:46:22Z

@phsft-bot build please

phsft-bot · 2021-02-19T13:46:28Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

eguiraud

Building one last time before merging, but I think this is good to go. root-project/roottest#660 updates roottest_root_multicore_tp_process_imt which fails with this patch.

phsft-bot · 2021-02-19T14:27:16Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-4.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-19T14:53:54Z

Build failed on mac1014/python3.
Running on macphsft17.dyndns.cern.ch:/build/jenkins/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-19T15:13:50Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on null:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-19T16:02:39Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-19T16:05:40Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-19T16:19:05Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.multicore.roottest_root_multicore_tp_process_imt

phsft-bot · 2021-02-19T19:38:58Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

eguiraud · 2021-02-19T21:23:05Z

Thank you very much @bendavid !

bendavid requested a review from pcanal as a code owner January 27, 2021 17:18

oshadura assigned pcanal Jan 29, 2021

eguiraud assigned eguiraud and unassigned pcanal Jan 29, 2021

eguiraud requested review from eguiraud and removed request for pcanal January 29, 2021 14:01

eguiraud reviewed Jan 29, 2021

View reviewed changes

pcanal reviewed Jan 30, 2021

View reviewed changes

tree/treeplayer/src/TTreeProcessorMT.cxx Outdated Show resolved Hide resolved

set TasksPerWorkerHint rather than MaxTasksPerFilePerWorker

83e848b

bendavid force-pushed the improvetasksplitting branch from b30fabe to 83e848b Compare February 16, 2021 18:57

[TreeProcMT][NFC] Improve docs

371c196

eguiraud added 2 commits February 17, 2021 18:53

[TreeProcMT] Deprecate {G,S}etMaxTasksPerFilePerWorker

dd6898a

[TreeProcMT][NFC] Formatting

02c0826

etejedor reviewed Feb 18, 2021

View reviewed changes

tree/treeplayer/src/TTreeProcessorMT.cxx Outdated Show resolved Hide resolved

etejedor reviewed Feb 18, 2021

View reviewed changes

tree/treeplayer/src/TTreeProcessorMT.cxx Outdated Show resolved Hide resolved

[TreeProcMT][NFC] Improve comments

32ad2f7

Co-authored-by: Enric Tejedor Saavedra <[email protected]>

eguiraud mentioned this pull request Feb 19, 2021

Remove usage of deprecated SetMaxTasksPerFilePerWorker root-project/roottest#660

Merged

eguiraud approved these changes Feb 19, 2021

View reviewed changes

eguiraud merged commit b7e8bb7 into root-project:master Feb 19, 2021

bendavid mentioned this pull request Mar 16, 2021

Further improvements to reduce lock contention #6919

Closed

Conversation

bendavid commented Jan 27, 2021

Uh oh!

phsft-bot commented Jan 27, 2021

Uh oh!

bendavid commented Jan 27, 2021

Uh oh!

oshadura commented Jan 29, 2021

Uh oh!

phsft-bot commented Jan 29, 2021

Uh oh!

phsft-bot commented Jan 29, 2021

Failing tests:

Uh oh!

phsft-bot commented Jan 29, 2021

Failing tests:

Uh oh!

phsft-bot commented Jan 29, 2021

Failing tests:

Uh oh!

phsft-bot commented Jan 29, 2021

Failing tests:

Uh oh!

phsft-bot commented Jan 29, 2021

Failing tests:

Uh oh!

phsft-bot commented Jan 29, 2021

Failing tests:

Uh oh!

eguiraud commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Jan 29, 2021

Uh oh!

eguiraud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eguiraud commented Feb 1, 2021

Uh oh!

bendavid commented Feb 1, 2021

Uh oh!

eguiraud commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendavid commented Feb 1, 2021

Uh oh!

eguiraud commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eguiraud commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendavid commented Feb 16, 2021

Uh oh!

eguiraud commented Feb 17, 2021

Uh oh!

phsft-bot commented Feb 17, 2021

Uh oh!

phsft-bot commented Feb 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Feb 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Feb 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Feb 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Feb 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Feb 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Feb 17, 2021

Errors:

Uh oh!

eguiraud commented Jan 29, 2021 •

edited

Loading

eguiraud commented Feb 1, 2021 •

edited

Loading

eguiraud commented Feb 1, 2021 •

edited

Loading

eguiraud commented Feb 8, 2021 •

edited

Loading