root-project
[cxxmodules][cling] Avoid loading some unnecessary modules
When we run into an unkown identifier that is a namespace, we don't really need to load its corresponding modules. Instead, we create a new module that forward declared all namespaces and always load it first. By doing so, we can avoid loading a lot of unnecessary modules.
Signed-off-by: Jun Zhang [email protected]
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
- Update the commit message
- Fix
Datais not initialized We need to another build and test @vgvassilev
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
@davidlange6, can we pick up this PR in the CXXMODULES IB and test if we bring down memory footprint?
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
After b544a58, the size of modules.idx almost grows 5 times larger:
master branch:
❯ du -h lib/modules.idx
173K lib/modules.idx
after 44434f0 (without the fix):
❯ du -h lib/modules.idx
185K lib/modules.idx
after b544a58 (with the fix):
❯ du -h lib/modules.idx
841K lib/modules.idx
But consider it still under 1 MB, and relatively small compared to other stuff like libCling.so (323M), I think we can afford it.
Maybe only register function declarations, I can see it reduce the size from 841K to 549K...
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test...
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test...
Thanks!
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test...
Thank you, David! BTW how can I see the test result?
It wasn't an automatic test, I just have logs of memory used by an example job.
On Jul 12, 2022, at 12:14 PM, Jun Zhang @.***> wrote:
@vgvassilev - I added this patch for tonight's cmssw modules IB for a test...
Thank you, David! BTW how can I see the test result?
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.
@smuzaffar, can we trigger a cmssw IB for this change. I doubt there will be any breakage but let's just make sure that's the case.
If we only care about if the identifier we look up is a top-level namespace or not, maybe we don't need to store the DeckKind info in GlobalModuleIndex, I plan to rework a little bit and only use a char to represent the state. I believe it decreases modules.idx size a little bit as well. WDYT? @vgvassilev
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
If we only care about if the identifier we look up is a top-level namespace or not, maybe we don't need to store the
DeckKindinfo in GlobalModuleIndex, I plan to rework a little bit and only use acharto represent the state. I believe it decreasesmodules.idxsize a little bit as well. WDYT? @vgvassilev
We care if the identifier is a namespace. The top namespace is just a heuristic which we use now. However, I expect to be able to avoid loading of all namespaces until another declaration is needed.
Thanks for working on this! Let's aim to merge this PR - I have added several comments.
We might be thinking how to make the patch in clang's GlobalModuleIndex smaller or orthogonal to the existing functionality as we might be breaking some upstream use cases.
Now it decreased to 345K
❯ du -h lib/modules.idx
345K lib/modules.idx
If we only care about if the identifier we look up is a top-level namespace or not, maybe we don't need to store the
DeckKindinfo in GlobalModuleIndex, I plan to rework a little bit and only use acharto represent the state. I believe it decreasesmodules.idxsize a little bit as well. WDYT? @vgvassilevWe care if the identifier is a namespace. The top namespace is just a heuristic which we use now. However, I expect to be able to avoid loading of all namespaces until another declaration is needed.
OK, but this may not only need just record some flags in the GMI but also other work. For now, I think the implementation is good enough.
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
Build failed on mac1015/cxx17. Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build See console output.
Failing tests:
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds
Build failed on ROOT-performance-centos8-multicore/cxx17. Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build See console output.
Failing tests:
- projectroot.runtutorials.tutorial_legacy_rootenv
- projectroot.runtutorials.tutorial_roostats_rs_numbercountingutils
Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds