Skip to content

Conversation

@Axel-Naumann
Copy link
Member

Instead of looking for libraries resolving each unresolved symbol,
just load each lib resolving symbols: this will allow subsequent
missing symbols to be resolved from the loaded library quickly,
without touching disk.

When loading an existing ACLiC library, and if we expect it
to contain the dependencies (explicit linking), just load itt,
instead of trying to (re-)determine its dependencies from its
undefined symbols: the outcome should be just the library
dependencies we expect the library to know already.

@Axel-Naumann Axel-Naumann self-assigned this Apr 27, 2021
@Axel-Naumann
Copy link
Member Author

I'll clean up git history once the tests pass - there's root-project/roottest#699 and the very different projectroot.core.metacling.test.gtest_core_metacling_test_TClingTest

@Axel-Naumann
Copy link
Member Author

Axel-Naumann commented Oct 28, 2021

Loading a library that is needed to resolve symbols (as done by this PR) means that libraries linked against that library are loaded as a side-effect, without TCling / TSystem seeing it. TClingTest's gInterpreter->GetSharedLibDeps("libTreePlayer") thus doesn't report all actual library dependencies but only a subset. This isn't an issue for explicit linking, though (neither for implicit linking, which will load all libraries separately and thus enumerate them), but it's a potential correctness concern and an inconsistency for explicit vs implicit linking / loading.

@Axel-Naumann Axel-Naumann marked this pull request as ready for review November 2, 2021 16:14
@Axel-Naumann Axel-Naumann requested a review from pcanal November 2, 2021 16:14
@Axel-Naumann Axel-Naumann force-pushed the ACLiC-do-not-walk-all-libs-for-U-syms branch 2 times, most recently from 8c1ed58 to c08b2da Compare November 3, 2021 18:04
When loading an existing ACLiC library, and if we expect it
to contain the dependencies (explicit linking), just load it,
instead of trying to (re-)determine its dependencies from its
undefined symbols: the outcome should be just the library
dependencies we expect the library to know already.

For the (rare) non-explicit-linking case, continue to find and
load all dependent libraries. Way slower, but also way rarer.
@Axel-Naumann Axel-Naumann force-pushed the ACLiC-do-not-walk-all-libs-for-U-syms branch from c08b2da to ed10012 Compare November 4, 2021 07:29
if (!keep) k->SetBit(kMustCleanup);
fCompiled->Add(k);

gInterpreter->GetSharedLibDeps(library);
Copy link
Member

@pcanal pcanal Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call is still needed for the not-explicitly-linked case. It has the side effect of load the dependencies from the rootmap files so that the subsequent LoadLibrary can use that information. See commit 8fdd5c3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related we should probably deprecate the option to disable explicit linking in ACLiC (it is already removed from the main build). Eitherway removing this line probably require to add (or check we already have) a test for the problem solved by commit 8fdd5c3

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved but need one more correction.

@ferdymercury
Copy link
Collaborator

ferdymercury commented Feb 15, 2024

Other speedup related issues:
#14287
#7123
https://its.cern.ch/jira/browse/ROOT-8848

@github-actions
Copy link

github-actions bot commented Sep 24, 2024

Test Results

    17 files      17 suites   4d 3h 48m 11s ⏱️
 2 694 tests  2 694 ✅ 0 💤 0 ❌
44 129 runs  44 129 ✅ 0 💤 0 ❌

Results for commit ed10012.

♻️ This comment has been updated with latest results.

@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from Axel-Naumann Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from Axel-Naumann Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@root-project root-project deleted a comment from phsft-bot Jan 2, 2025
@guitargeek guitargeek closed this Jan 2, 2025
@guitargeek guitargeek reopened this Jan 2, 2025
@ferdymercury ferdymercury reopened this Jan 13, 2025
@ferdymercury ferdymercury requested a review from hahnjo September 19, 2025 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants