Introduce PagedVector class #13808

ktf · 2023-10-05T11:50:17Z

The goal of the class is to be an (almost) drop in replacement for SmallVector and std::vector when those are presized and filled later, as it happens in SourceManager and ASTReader.

By splitting the actual vector in pages of the same size and allocating the pages only when they are needed, using this containers reduces the memory usage by a factor 4 for the cases relevant to the ALICE experiment ROOT / cling usage.

phsft-bot · 2023-10-05T11:50:24Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

github-actions · 2023-10-05T13:34:07Z

Test Results

9 files 9 suites 1d 20h 22m 36s ⏱️
2 490 tests 2 490 ✅ 0 💤 0 ❌
21 378 runs 21 378 ✅ 0 💤 0 ❌

Results for commit b99836b.

♻️ This comment has been updated with latest results.

vgvassilev · 2023-10-06T06:01:55Z

@ktf, this is a backport of llvm/llvm-project#66430 but does it include llvm/llvm-project#67958?

hahnjo · 2023-10-06T07:59:01Z

@ktf this fails quite horribly in CI:

TFormulaGradientTests: /github/home/ROOT-CI/src/interpreter/llvm-project/clang/include/clang/Basic/SourceManager.h:1694: const clang::SrcMgr::SLocEntry& clang::SourceManager::getLocalSLocEntry(unsigned int) const: Assertion `Index < LocalSLocEntryTable.size() && "Invalid index"' failed.

@Axel-Naumann I'm not convinced this should be backported to 6.28, this has the potential to break a lot of things...

Axel-Naumann · 2023-10-06T08:21:37Z

With the original PR accepted upstream and an experiment requesting this we need good reasons (such as "fails quite horribly in CI" :-) ) to refuse the backport. I think we want to work with @ktf to make this PR work...

@ktf the problem is that another patch release is imminent and this PR seems not yet ready for prime time. It will have to miss this train but take the next one.

vgvassilev · 2023-10-06T08:42:16Z

Looks like we fail with TFormulaGradientTests: /github/home/ROOT-CI/src/interpreter/llvm-project/clang/include/clang/Basic/SourceManager.h:1694: const clang::SrcMgr::SLocEntry& clang::SourceManager::getLocalSLocEntry(unsigned int) const: Assertion `Index < LocalSLocEntryTable.size() && "Invalid index"' failed.

vgvassilev · 2023-10-06T08:44:10Z

@hahnjo, the area in clang which @ktf touches hasn't been changing for years. Maybe we just discovered something that upstream has not and we need to tune the patch...

hahnjo · 2023-10-06T08:45:49Z

Looks like we fail with [...]

Yes, that's exactly what I posted 45 minutes ago...

@hahnjo, the area in clang which @ktf touches hasn't been changing for years. Maybe we just discovered something that upstream has not and we need to tune the patch...

Yes, that's what I'm saying. And IMHO that "tuning" should not happen after we broke everybody's ROOT in a minor patch release...

vgvassilev · 2023-10-06T08:53:02Z

Looks like we fail with [...]

Yes, that's exactly what I posted 45 minutes ago...

@hahnjo, the area in clang which @ktf touches hasn't been changing for years. Maybe we just discovered something that upstream has not and we need to tune the patch...

Yes, that's what I'm saying. And IMHO that "tuning" should not happen after we broke everybody's ROOT in a minor patch release...

Agreed, let's fix the failures in the master and re-evaluate what should be done next.

ktf · 2023-10-06T08:59:47Z

I am checking what's going on. I might have simply screwed up the backport.

ktf · 2023-10-06T09:36:11Z

@ktf, this is a backport of llvm/llvm-project#66430 but does it include llvm/llvm-project#67958?

Not yet.

ktf · 2023-10-06T09:37:12Z

@ktf the problem is that another patch release is imminent and this PR seems not yet ready for prime time. It will have to miss this train but take the next one.

That's fine for me.

ktf · 2023-10-06T11:00:39Z

Needless to say that the (+ -DLLVM_ENABLE_ASSERTIONS=On) with my setup works fine. Investigating what difference I have in the build environment (I also already tried CXX20...).

phsft-bot · 2023-10-06T12:09:16Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

phsft-bot · 2023-10-06T12:10:45Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

ktf · 2023-10-06T12:11:31Z

The updated version should be closer to what is currently in llvm upstream...

hahnjo · 2023-10-06T13:04:58Z

The updated version should be closer to what is currently in llvm upstream...

What are the differences? Ideally we should apply the exact patch from upstream, it should cleanly go away on a future LLVM upgrade...

ktf · 2023-10-06T14:04:48Z

Apparently ROOT's llvm-project is not up-to-date.

hahnjo · 2023-10-10T12:53:51Z

Apparently ROOT's llvm-project is not up-to-date.

https://github.com/root-project/llvm-project/ matches what is currently in master. In this PR, you are modifying files in interpreter/llvm-project/, so of course these changes cannot be reflected in https://github.com/root-project/llvm-project/. This needs to be synchronized eventually, but first the test failures need to be addressed (across all platforms).

vgvassilev · 2023-10-19T15:00:12Z

ping...

vgvassilev · 2023-11-01T07:21:05Z

@ktf ping...

phsft-bot · 2024-01-08T10:30:45Z

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

Errors:

[2024-01-08T10:30:44.430Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1126 (message):

hahnjo · 2024-01-08T10:32:43Z

I believe you have to rebase, not merge the master branch...

phsft-bot · 2024-01-08T11:22:20Z

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

[2024-01-08T11:22:19.257Z] CMake Error at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1126 (message):

phsft-bot · 2024-01-08T11:25:27Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2024-01-08T11:25:22.212Z] stderr: error: Failed to merge in the changes.
[2024-01-08T11:25:26.936Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1126 (message):

phsft-bot · 2024-01-08T11:50:43Z

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2024-01-08T11:50:42.692Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1126 (message):

phsft-bot · 2024-01-08T11:57:07Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

phsft-bot · 2024-01-08T13:38:18Z

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

Failing tests:

hahnjo · 2024-01-08T13:39:50Z

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=ON

phsft-bot · 2024-01-08T13:39:57Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default with flags -DCTEST_TEST_EXCLUDE_NONE=ON
How to customize builds

phsft-bot · 2024-01-08T15:26:36Z

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

Failing tests:

The goal of the class is to be an (almost) drop in replacement for SmallVector and std::vector when those are presized and filled later, as it happens in SourceManager and ASTReader. By doing so, sparsely accessed PagedVector can profit from reduced memory footprint. Co-authored-by: Jonas Hahnfeld <[email protected]>

phsft-bot · 2024-01-16T08:43:39Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

hahnjo · 2024-01-16T08:55:07Z

Hi @ktf, as discussed yesterday the test failures are related to an issue in our incremental builds where Clad is not rebuilt after changes to the Clang headers. This leads to very weird symptoms because some "stale" functions access memory where they shouldn't and so on. I was hit by this problem already twice and it's tracked in #7977, so one would suppose that I remember by now but evidently I didn't... Apologies for the confusion and the delay it caused in integrating this.

I've now synchronized the changes to https://github.com/root-project/llvm-project/releases/tag/ROOT-llvm16-20240116-01, moving the header to clang/include/clang/Basic as mentioned yesterday to keep the ability to build against a vanilla version of LLVM). @vgvassilev I put the commit only into ROOT-llvm16, not cling-llvm16 because I think it's not that relevant for Cling standalone. Let me know if you disagree and I can of course move it.

Some measurements of this change on my machine: for a simple ./bin/root.exe -q, it reduces the maximum RSS from 217MB to 192MB and for ./bin/root.exe -q -e "std::vector<int> v" -e "return 0;" from 255MB to 230MB 👏

ktf · 2024-01-16T09:23:36Z

Very nice, thank you for your followup. Shall I close this one?

ktf · 2024-01-16T09:32:05Z

BTW, I think you can move IdentifiersLoaded to be a paged vector as well, although one would need to measure the effect on RSS.

hahnjo · 2024-01-16T09:44:00Z

Very nice, thank you for your followup. Shall I close this one?

No, please leave it open: I hijacked your branch and directly pushed the slightly modified commit there. It's currently running through our CI (which is a bit congested at the moment). Once all is green, we can merge 😃

BTW, I think you can move IdentifiersLoaded to be a paged vector as well, although one would need to measure the effect on RSS.

Ok, that's for a follow-up PR I would say.

vgvassilev

LGTM!

hahnjo · 2024-01-23T08:16:24Z

Hi @ktf, if we were to backport this to a release, which one would ALICE be interested in, 6.30? Or would you be fine keeping the patch locally in your stack for now?

ktf · 2024-01-23T08:46:40Z

Yes, we would need 6.30, but for now we are fine.

hahnjo · 2024-01-23T15:16:03Z

Yes, we would need 6.30, but for now we are fine.

Ok, I prepared #14411. To be seen if we already include it in 6.30.04 or only in .06

ktf · 2024-01-24T08:28:36Z

Perfect, thanks.

ktf requested a review from vgvassilev as a code owner October 5, 2023 11:50

Axel-Naumann modified the milestones: 6.30/00, 6.28/06 Oct 5, 2023

bellenot assigned vgvassilev Oct 5, 2023

hahnjo self-requested a review October 6, 2023 07:57

Axel-Naumann removed this from the 6.28/06 milestone Oct 6, 2023

ktf force-pushed the reduce-memory-usage branch from 8785b55 to d6a8bd9 Compare October 6, 2023 12:09

ktf force-pushed the reduce-memory-usage branch from d6a8bd9 to 27fa1fb Compare October 6, 2023 12:10

hahnjo force-pushed the reduce-memory-usage branch from 39c742c to 7c2fde0 Compare January 8, 2024 11:57

hahnjo added the clean build Ask CI to do non-incremental build on PR label Jan 16, 2024

hahnjo self-assigned this Jan 16, 2024

hahnjo force-pushed the reduce-memory-usage branch from 7c2fde0 to b99836b Compare January 16, 2024 08:43

hahnjo requested review from vgvassilev and removed request for vgvassilev January 17, 2024 07:24

vgvassilev approved these changes Jan 17, 2024

View reviewed changes

hahnjo merged commit 9c7bac0 into root-project:master Jan 18, 2024

ktf deleted the reduce-memory-usage branch January 23, 2024 08:46

hahnjo mentioned this pull request Jan 23, 2024

[llvm-project] Introduce paged vector [v6.30] #14411

Merged

Introduce PagedVector class #13808

Introduce PagedVector class #13808

Conversation

ktf commented Oct 5, 2023

Uh oh!

phsft-bot commented Oct 5, 2023

Uh oh!

github-actions bot commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

vgvassilev commented Oct 6, 2023

Uh oh!

hahnjo commented Oct 6, 2023

Uh oh!

Axel-Naumann commented Oct 6, 2023

Uh oh!

vgvassilev commented Oct 6, 2023

Uh oh!

vgvassilev commented Oct 6, 2023

Uh oh!

hahnjo commented Oct 6, 2023

Uh oh!

vgvassilev commented Oct 6, 2023

Uh oh!

ktf commented Oct 6, 2023

Uh oh!

ktf commented Oct 6, 2023

Uh oh!

ktf commented Oct 6, 2023

Uh oh!

ktf commented Oct 6, 2023

Uh oh!

phsft-bot commented Oct 6, 2023

Uh oh!

phsft-bot commented Oct 6, 2023

Uh oh!

ktf commented Oct 6, 2023

Uh oh!

hahnjo commented Oct 6, 2023

Uh oh!

ktf commented Oct 6, 2023

Uh oh!

hahnjo commented Oct 10, 2023

Uh oh!

vgvassilev commented Oct 19, 2023

Uh oh!

vgvassilev commented Nov 1, 2023

Uh oh!

phsft-bot commented Jan 8, 2024

Errors:

Uh oh!

hahnjo commented Jan 8, 2024

Uh oh!

phsft-bot commented Jan 8, 2024

Errors:

Uh oh!

phsft-bot commented Jan 8, 2024

Errors:

Uh oh!

phsft-bot commented Jan 8, 2024

Errors:

Uh oh!

phsft-bot commented Jan 8, 2024

Uh oh!

phsft-bot commented Jan 8, 2024

Failing tests:

Uh oh!

hahnjo commented Jan 8, 2024

Uh oh!

phsft-bot commented Jan 8, 2024

Uh oh!

phsft-bot commented Jan 8, 2024

Failing tests:

Uh oh!

phsft-bot commented Jan 16, 2024

Uh oh!

hahnjo commented Jan 16, 2024

Uh oh!

ktf commented Jan 16, 2024

github-actions bot commented Oct 5, 2023 •

edited

Loading