[ntuple] Reduce memory usage of RPageSinkBuf
#20425
Conversation
Created tasks reference *this, so moving is not safe. It's also not needed because RPageSinkBuf is always inside a std::unique_ptr.
When IMT is turned on and RPageSinkBuf has an RTaskScheduler, we would previously buffer all pages and create tasks to seal / compress them. While this exposes the maximum work, it's a waste of memory if other threads are not fast enough to process the tasks. Heuristically assume that there is enough work if we already buffer more uncompressed bytes than the approximate zipped cluster size. In a small test, writing random data with ROOT::EnableImplicitMT(1) and therefore no extra worker thread, the application used 500 MB before this change for the default cluster size of 128 MiB. After this change, memory usage is reduced to around 430 MB (compared to a memory usage of 360 MB without IMT). The compression factor is around ~2.1x in this case, which roughly checks out: Instead of buffering the full uncompressed cluster (which is around compression factor * zipped cluster size = 270 MiB), we now buffer uncompressed pages up to the approximate zipped cluster size (128 MiB) and then start compressing pages immediately. The result of course also needs to be buffered, but is much smaller after compression: ((1 - 1 / compression factor) * zipped cluster size = 67 MiB). Accordingly, the gain will be higher for larger compression factors.
hahnjo
requested review from
enirolf,
jblomer,
pcanal,
silverweed and
vepadulano
Test Results 22 files 22 suites 3d 14h 15m 11s ⏱️ Results for commit e896cd5. |
|
Thanks @hahnjo! Just thinking out loud how we would eventually test the impact of this PR, is this something you'd be comfortable in backporting to 6.36, or would you prefer to keep it in |
Hi @makortel, in the past I rebuilt ROOT locally with the same configuration as in the IB (in particular |
|
Thanks @hahnjo. Sounds like we should evaluate the impact through the CMSSW IB that tracks ROOT master (can be done). |
|
That testing will of course also get much easier once you move to ROOT 6.36 globally and merge the RNTuple modules into |
3 participants
When IMT is turned on and
RPageSinkBufhas anRTaskScheduler, we would previously buffer all pages and create tasks to seal / compress them. While this exposes the maximum work, it's a waste of memory if other threads are not fast enough to process the tasks. Heuristically assume that there is enough work if we already buffer more uncompressed bytes than the approximate zipped cluster size.In a small test, writing random data with
ROOT::EnableImplicitMT(1)and therefore no extra worker thread, the application used 500 MB before this change for the default cluster size of 128 MiB. After this change, memory usage is reduced to around 430 MB (compared to a memory usage of 360 MB without IMT). The compression factor is around ~2.1x in this case, which roughly checks out:Instead of buffering the full uncompressed cluster (which is around compression factor * zipped cluster size = 270 MiB), we now buffer uncompressed pages up to the approximate zipped cluster size (128 MiB) and then start compressing pages immediately. The result of course also needs to be buffered, but is much smaller after compression: ((1 - 1 / compression factor) * zipped cluster size = 67 MiB). Accordingly, the gain will be higher for larger compression factors.
Closes #18314
FYI @Dr15Jones @makortel as discussed previously