v6-30 Backport cling fixes for CMS #14358

vepadulano · 2024-01-16T13:33:39Z

Backports of #14287 and #14352

phsft-bot · 2024-01-16T13:33:47Z

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-16T13:40:22Z

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2024-01-16T13:36:46.790Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/metacling/src/TClingCallbacks.cxx:90:120: error: cannot allocate an object of abstract type ‘llvm::orc::MaterializationUnit’

dpiparo

if tests pass, we can merge.

vepadulano · 2024-01-16T13:55:56Z

Unfortunately the commits do not even compile with some scary error

 /home/sftnight/build/workspace/root-pullrequests-build/root/core/metacling/src/TClingCallbacks.cxx: In constructor ‘AutoloadLibraryMU::AutoloadLibraryMU(const TClingCallbacks&, const string&, const SymbolNameVector&)’:
/home/sftnight/build/workspace/root-pullrequests-build/root/core/metacling/src/TClingCallbacks.cxx:90:120: error: cannot allocate an object of abstract type ‘llvm::orc::MaterializationUnit’
   90 |       : MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)

Can this be some difference between LLVM 13 and 16 that's causing it? @vgvassilev

vgvassilev · 2024-01-16T13:59:50Z

Nice we need to re-solve this for 6.30...

vgvassilev · 2024-01-16T14:01:15Z

core/metacling/src/TClingCallbacks.cxx

-   AutoloadLibraryMU(const std::string &Library, const llvm::orc::SymbolNameVector &Symbols)
-      : MaterializationUnit(getSymbolFlagsMap(Symbols), nullptr), fLibrary(Library), fSymbols(Symbols)
+   AutoloadLibraryMU(const TClingCallbacks &cb, const std::string &Library, const llvm::orc::SymbolNameVector &Symbols)
+      : MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)


Suggested change

: MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)

: MaterializationUnit(getSymbolFlagsMap(Symbols), nullptr), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)

How about that one.

The build finishes fine on my machine, I have updated the commit

phsft-bot · 2024-01-16T14:12:13Z

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

Errors:

[2024-01-16T14:06:48.405Z] C:\build\workspace\root-pullrequests-build\root\core\metacling\src\TClingCallbacks.cxx(90,29): error C2259: 'llvm::orc::MaterializationUnit': cannot instantiate abstract class [C:\build\workspace\root-pullrequests-build\build\core\metacling\src\MetaCling.vcxproj]

github-actions · 2024-01-16T14:20:17Z

Test Results

9 files 9 suites 1d 11h 20m 11s ⏱️
2 481 tests 2 479 ✅ 0 💤 2 ❌
21 361 runs 21 359 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit d4f8f6a.

♻️ This comment has been updated with latest results.

phsft-bot · 2024-01-16T14:27:23Z

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

smuzaffar · 2024-01-16T14:27:34Z

thanks for the backport, I have started CMSSW tests here cms-sw#195

vepadulano · 2024-01-16T14:30:30Z

thanks for the backport, I have started CMSSW tests here cms-sw#195

Hi @smuzaffar , thanks for your promptness! We discovered there was a small wrinkle preventing proper compilation in 6.30, I have uploaded an updated commit which should work properly literally a few minutes ago. ~~I just wanted to make sure that the PR on CMSSW takes this latest version of the commits~~

EDIT: I checked on the linked CMSSW PR and it seems the right version of the commit was picked, so should be fine.

phsft-bot · 2024-01-16T15:01:07Z

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.core.metacling.test.gtest_core_metacling_test_TClingTest

vepadulano · 2024-01-16T15:44:29Z

Ok now it compiles but we get the following error in the new test

  input_line_49:1:21: error: unknown type name 'ROOT_7459'
  int ROOT_7459 = 42; ROOT_7459++;
                      ^
  input_line_49:1:30: error: expected unqualified-id
  int ROOT_7459 = 42; ROOT_7459++;

phsft-bot · 2024-01-16T15:50:54Z

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

Failing tests:

projectroot.core.metacling.test.gtest_core_metacling_test_TClingTest

dpiparo · 2024-01-16T16:26:46Z

I would like to point out that this is a backport of a very generic fix, which works very well with llvm16, which is not in the branch 6.30. A solution of the issue reported by CMS was already made available in December: if you want, this is a more refined version of that.

vgvassilev · 2024-01-16T16:30:31Z

Ok now it compiles but we get the following error in the new test

  input_line_49:1:21: error: unknown type name 'ROOT_7459'
  int ROOT_7459 = 42; ROOT_7459++;
                      ^
  input_line_49:1:30: error: expected unqualified-id
  int ROOT_7459 = 42; ROOT_7459++;

Somehow the parsing seems broken. We should probably try using ProcessLine instead of Declare... still a mystery why this fails only for the PCH based builds...

EDIT: Parsing is broken because Declare does not support statements on the global scope but ProcessLine does. In addition it works in master because clang already supports statements on the global scope (thanks to clang-repl)...

vepadulano · 2024-01-16T16:41:29Z

We should probably try using ProcessLine instead of Declare

Seems like an easy test, let me try that 👍

phsft-bot · 2024-01-16T17:14:09Z

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

vepadulano · 2024-01-16T17:14:35Z

At least on my machine substituting Declare with ProcessLine fixed the test failure, let's see what the CI has to say

vgvassilev · 2024-01-16T17:55:28Z

At least on my machine substituting Declare with ProcessLine fixed the test failure, let's see what the CI has to say

We need to land that change in the master for consistency reasons.

vepadulano · 2024-01-16T18:53:41Z

We need to land that change in the master for consistency reasons.

Good point! I will then separate this change from the original commit and create a separate commit that we can forward-port to master

The llvm9 JIT issued callbacks when a symbol was missing and we reacted on it by loading the relevant library. In root-project/root@9b2041e3 we have kept the logic but now the JIT started querying more often even for symbols which are okay to be missing. In turn that leads to scanning all libraries causing performance issues. This patch tries to limit this functionality only in contexts where automatic loading is allowed.

Co-authored-by: Jonas Hahnfeld <jonas.hahnfeld@cern.ch>

Symbol lookup is a quite expensive operation and might result in JIT compilation and library loading. Co-authored-by: Jonas Hahnfeld <jonas.hahnfeld@cern.ch>

This reverts commit 329fb5a.

This allows us to avoid generating symbols in libCore for these constants keeping the same amount of open calls at ROOT startup time.

`TInterpreter::Declare` does not support issuing statements on the global scope, while `TInterpreter::ProcessLine` does. This test was first introduced in a development version of ROOT (6.31), where the upgrade to LLVM16 was already in place. In that scenario, clang supports global scope statements (thanks to changes made for clang-repl). Applying the test to ROOT 6.30 uncovered the problem, since the clang of LLVM13 does not support global scope statements. Fix the test by using `ProcessLine` instead of `Declare` which does the intended thing independently from the ROOT version. Co-authored-by: Vassil Vassilev <vasil.georgiev.vasilev@cern.ch>

phsft-bot · 2024-01-16T20:56:30Z

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

vepadulano · 2024-01-17T08:10:00Z

@vgvassilev The test now passes on all platforms. Let me know what you think about the commit and if we can merge this PR

vgvassilev

LGTM!

dpiparo · 2024-01-17T16:08:11Z

Dear @smuzaffar , also thanks to your help, we converged. This PR merged the changes that for us are an improvement wrt the fix provided in December 2023. This the commit we would propose to CMS for the CMSSW 14 release: would you be so kind to give it another try to verify once again the code works as expected for CMS?

smuzaffar · 2024-01-17T16:49:30Z

@dpiparo , thanks for the backport. CMSSW tests via cms-sw#195 for this change look good. I just have opened cms-sw/cmsdist#8949 to integrate latest v6-30-00-patches changes in to CMSSW 14.0.X

phsft-bot · 2024-01-17T19:16:56Z

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-17T20:04:22Z

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.tmva.sofie.test.gtest_tmva_sofie_test_TestSofieModels

phsft-bot · 2024-01-17T20:12:23Z

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

Failing tests:

projectroot.bindings.pyroot.pythonizations.test.pyunittests_pyroot_dependency_versions

vepadulano added the pr:backport label Jan 16, 2024

vepadulano self-assigned this Jan 16, 2024

vepadulano requested review from Axel-Naumann and pcanal as code owners January 16, 2024 13:33

dpiparo self-requested a review January 16, 2024 13:54

dpiparo approved these changes Jan 16, 2024

View reviewed changes

dpiparo mentioned this pull request Jan 16, 2024

Large number of file open during unit test testMakeTrackValidationPlots in ROOT628 cms-sw/cmssw#43077

Closed

vgvassilev reviewed Jan 16, 2024

View reviewed changes

vepadulano force-pushed the backport-fixes-for-cms branch from 33307cb to 583c720 Compare January 16, 2024 14:27

vepadulano force-pushed the backport-fixes-for-cms branch from 583c720 to 4504241 Compare January 16, 2024 17:14

vgvassilev and others added 5 commits January 16, 2024 21:49

[core] Evaluate initializer of constant variables

ce9bbc8

Co-authored-by: Jonas Hahnfeld <jonas.hahnfeld@cern.ch>

[core] Prefer variable initializer over symbol search

a2be28e

Symbol lookup is a quite expensive operation and might result in JIT compilation and library loading. Co-authored-by: Jonas Hahnfeld <jonas.hahnfeld@cern.ch>

Revert "[core] Materialize symbols for TError variables"

14cc85c

This reverts commit 329fb5a.

[core] Make some constants constexpr

284c5d5

This allows us to avoid generating symbols in libCore for these constants keeping the same amount of open calls at ROOT startup time.

vepadulano force-pushed the backport-fixes-for-cms branch from 4504241 to d4f8f6a Compare January 16, 2024 20:56

vgvassilev approved these changes Jan 17, 2024

View reviewed changes

vepadulano merged commit be7addb into root-project:v6-30-00-patches Jan 17, 2024
10 of 15 checks passed

pcanal changed the title ~~Backport cling fixes for CMS~~ v6-30 Backport cling fixes for CMS Jan 17, 2024

dpiparo mentioned this pull request Jan 24, 2024

v6-28 Backport cling fixes for ATLAS #14423

Merged

vepadulano mentioned this pull request Jan 25, 2024

Fix statements at global scope in TCling test #14435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v6-30 Backport cling fixes for CMS #14358

v6-30 Backport cling fixes for CMS #14358

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

dpiparo left a comment

vepadulano commented Jan 16, 2024

vgvassilev commented Jan 16, 2024

vgvassilev Jan 16, 2024

vepadulano Jan 16, 2024

vepadulano Jan 16, 2024

phsft-bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024 •

edited

Loading

phsft-bot commented Jan 16, 2024

smuzaffar commented Jan 16, 2024

vepadulano commented Jan 16, 2024 •

edited

Loading

phsft-bot commented Jan 16, 2024

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

dpiparo commented Jan 16, 2024

vgvassilev commented Jan 16, 2024 •

edited

Loading

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

vepadulano commented Jan 16, 2024

vgvassilev commented Jan 16, 2024

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

vepadulano commented Jan 17, 2024

vgvassilev left a comment

dpiparo commented Jan 17, 2024

smuzaffar commented Jan 17, 2024

phsft-bot commented Jan 17, 2024

phsft-bot commented Jan 17, 2024

phsft-bot commented Jan 17, 2024

	: MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)
	: MaterializationUnit(getSymbolFlagsMap(Symbols), nullptr), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)

v6-30 Backport cling fixes for CMS #14358

v6-30 Backport cling fixes for CMS #14358

Conversation

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

Errors:

dpiparo left a comment

Choose a reason for hiding this comment

vepadulano commented Jan 16, 2024

vgvassilev commented Jan 16, 2024

vgvassilev Jan 16, 2024

Choose a reason for hiding this comment

vepadulano Jan 16, 2024

Choose a reason for hiding this comment

vepadulano Jan 16, 2024

Choose a reason for hiding this comment

phsft-bot commented Jan 16, 2024

Errors:

github-actions bot commented Jan 16, 2024 • edited Loading

Test Results

phsft-bot commented Jan 16, 2024

smuzaffar commented Jan 16, 2024

vepadulano commented Jan 16, 2024 • edited Loading

phsft-bot commented Jan 16, 2024

Failing tests:

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

Failing tests:

dpiparo commented Jan 16, 2024

vgvassilev commented Jan 16, 2024 • edited Loading

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

vepadulano commented Jan 16, 2024

vgvassilev commented Jan 16, 2024

vepadulano commented Jan 16, 2024

phsft-bot commented Jan 16, 2024

vepadulano commented Jan 17, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

dpiparo commented Jan 17, 2024

smuzaffar commented Jan 17, 2024

phsft-bot commented Jan 17, 2024

phsft-bot commented Jan 17, 2024

Failing tests:

phsft-bot commented Jan 17, 2024

Failing tests:

github-actions bot commented Jan 16, 2024 •

edited

Loading

vepadulano commented Jan 16, 2024 •

edited

Loading

vgvassilev commented Jan 16, 2024 •

edited

Loading