[vepadulano]

Alternative to #22631 , implementing native IMT work-stealing for the RNTuple data source

[github-actions]

Test Results

    24 files      24 suites   4d 4h 39m 55s ⏱️
 3 893 tests  3 891 ✅ 0 💤 2 ❌
83 373 runs  83 371 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 20bebc0.

♻️ This comment has been updated with latest results.

[hageboeck]

🚀

Here's a first pass with a few suggestions and a few questions.

[jblomer]

Thank you for this work!

I think it may be easier to follow the code if the lambdas became class methods. I would also make documentation easier. Generally, I think a high-level documentation of the scheduling approach would be helpful.

Regarding the use of TBB: would it be possible/easier to simply use a TTaskGroup?

[vepadulano]

HI @hageboeck @jblomer , I have addressed your PR reviews and updated the changes. Documentation has been added throughout, and I've also memorialised the strategy in a separate markdown document as requested by @jblomer . Let me know your thoughts, thanks!

[jblomer]

Thanks!

Some first comments on the documentation. I'll review the code later today.

[vepadulano]

@jblomer thanks for the review! I have rebased the branch, updated the code to use the new LoadAllPageLists method to ensure clusters are available, added a test with many input files to check the data source keeps the number of open files limited. I have also implemented your comments to the parallel_scheduling markdown.

[jblomer]

In principle looks good to me. Some minor comments and one potentially larger questions on the work splitting.

[hahnjo]

Disclaimer: I've not carefully inspected every line in this PR, so feel free to point out if the following breaks down for very obvious reasons that I'm missing.

From a high level, it seems to me that "forcing" RNTuple scheduling into splittable ranges for consumption by TBB is both complex and limiting at the same time. The main "restriction" that we set ourselves is that we expect a created task A to work on file 1, task B on file 2, etc. I wonder if we can get more flexibility by just picking the next file in each task. This will also allow staging files in the background, done in the current implementation but likely not functional anymore in this PR (to be confirmed).

Roughly something like the following:

1. Start the background thread to stage files according to the number of slots.
2. Start as many tasks as files.
3. Upon execution, pick the next file and extend the sliding window of files to be staged.
4. Open the file and start iterating clusters (potentially with another loop for lazy loading of cluster group details).
5. When at some point, all queued tasks are executing (easily noticed by all files being picked up), go into tail-scheduling mode: create a (nested) task group and schedule tasks for all remaining clusters. Each cluster-task clones the page source from the "parent" file-task and then works on only that cluster. This should efficiently distribute the remaining work.

The number of open files should be limited to twice the number of slots:

• During staging, one per file-task and one per slot in the background thread.
• In the tail-scheduling mode, one per file-task and one for each executing cluster-task.

[vepadulano]

Thanks @jblomer for the review. After your comment on the parallel_scheduling.md file, about discussing the strategy to keep the number of open files checked, as well as the question on exactly one TBB may split a Range-based task, I spent some time studying TBB more carefully. I found the oneAPI docs a bit lacking, so I looked for other resources and found this book https://link.springer.com/chapter/10.1007/978-1-4842-4398-5_16. Chapter 9, pages 263 and onwards explain the splitting strategy quite clearly (at least to me). Splitting is done eagerly, as soon as one Range is created, it will be split recursively until only non-splittable sub-Range objects remain. These are then used concurrently as arguments to ParallelForBody. I also changed the logic to have the page source only be Cloned in the ParallelForBody, rather than within a Range. This in practice leads to a guarantee of at most 2*nSlots open page sources, which I've documented in the markdown and in code.

Thanks also to @hahnjo for the comment and the insightful conversation we had offline. Following that conversation, I agree that there is a generally more optimised strategy to be devised, which includes re-enabling the page source staging that this PR is currently disabling for the MT case.

I would propose to move ahead with the general logic proposed in this PR, as it shows better performance than the one currently available in master, and have this as the new MT strategy for 6.42. As a follow up, I will attempt to implement the strategy proposed by @hahnjo and @jblomer, and further refine it for the following ROOT release so that it will be also comparable with the strategy proposed by this PR.

[jblomer]

Fine for me to move forward with this now. I think it would be a good idea to open an issue where we summarize the subsequent planned changes.

Regarding the splitting: doesn't this mean that we end up with all tasks processing at most 3 clusters? This would mean that the cluster prefetching does not work very well because, by setting the entry range on the page source, we prevent prefetching across task boundaries.

We can probably work around this, but I'm less convinced now that TBB splittable tasks is what we want. I think for the next iteration, we should reconsider a design where we are in control of the exact task creation (e.g. along the lines of what @hahnjo suggests) together with a shared data structure that may require locking at every cluster boundary.

[vepadulano]

Regarding the splitting: doesn't this mean that we end up with almost all tasks processing 3 clusters plus the tail tasks with potentially fewer clusters? This would mean that the cluster prefetching does not work very well because, by setting the entry range on the page source, we prevent prefetching across task boundaries.

Yes, exactly. This is the main motivation for the stopping condition given by the IsDivisible method, having at least three clusters gives the page source some chance of prefetching the next clusters, although clearly this is less optimal for I/O purposes than reading the whole file, sequentially.

We can probably work around this, but I'm less convinced now that TBB splittable tasks is what we want. I think for the next iteration, we should reconsider a design where we are in control of the exact task creation (e.g. along the lines of what @hahnjo suggests) together with a shared data structure that may require locking at every cluster boundary.

We have discussed a strategy offline somewhat along those lines yes, the finer details will need to be seen in practice.