Skip to content

Conversation

@hahnjo
Copy link
Member

@hahnjo hahnjo commented Nov 18, 2024

Fixes commit e601bf8 and then removes the check from AutoloadLibraryMU (as I had requested in the original PR review).

Fixes commit e601bf8 ("[core] Reduce symbol search only to when
autoloading is enabled.").
This implements my request from the original PR review that autoloading
should only be checked in AutoloadLibraryGenerator but that the MU must
provide the symbols it promised. This wasn't working because the early
return was broken (fixed in the previous commit).
@hahnjo hahnjo requested a review from vgvassilev November 18, 2024 10:41
@hahnjo hahnjo self-assigned this Nov 18, 2024
@hahnjo
Copy link
Member Author

hahnjo commented Nov 18, 2024

Some numbers, following Vincenzo's methodology in #14287 (comment):

master

 $ time PYTHONPATH=lib/ python -c 'import ROOT;print(ROOT.gErrorIgnoreLevel);print(ROOT.kError)'
-1
3000

real    0m0.485s
user    0m0.384s
sys     0m0.099s
 $ PYTHONPATH=lib/ strace -e file python -c 'import ROOT;print(ROOT.gErrorIgnoreLevel);print(ROOT.kError)' 2>&1 | awk -v FS='"' '{ print $1 }' | sort | uniq -c
      1 -1
      1 3000
    163 access(
      2 chdir(
      1 execve(
      1 +++ exited with 0 +++
     94 getcwd(
    493 lstat(
    625 openat(AT_FDCWD, 
     92 readlink(
      1 --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=2429627, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
      1 --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=2429629, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
   8730 stat(
 $ PYTHONPATH=lib/ strace -z -f -o openat.log -e trace=open,openat python3 -c 'import ROOT;print(ROOT.gErrorIgnoreLevel);print(ROOT.kError)'
 $ grep openat openat.log | wc -l
453

cling-autoload

 $ time PYTHONPATH=lib/ python -c 'import ROOT;print(ROOT.gErrorIgnoreLevel);print(ROOT.kError)'
-1
3000

real    0m0.491s
user    0m0.387s
sys     0m0.103s
 $ PYTHONPATH=lib/ strace -e file python -c 'import ROOT;print(ROOT.gErrorIgnoreLevel);print(ROOT.kError)' 2>&1 | awk -v FS='"' '{ print $1 }' | sort | uniq -c
      1 -1
      1 3000
    163 access(
      2 chdir(
      1 execve(
      1 +++ exited with 0 +++
     94 getcwd(
    493 lstat(
    625 openat(AT_FDCWD, 
     92 readlink(
      1 --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=2441749, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
      1 --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=2441751, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
   8730 stat(
 $ PYTHONPATH=lib/ strace -z -f -o openat.log -e trace=open,openat python3 -c 'import ROOT;print(ROOT.gErrorIgnoreLevel);print(ROOT.kError)'
 $ grep openat openat.log | wc -l
453

@hahnjo
Copy link
Member Author

hahnjo commented Nov 18, 2024

I think this fix is relatively low-risk, but just to be sure: @aandvalenzuela @smuzaffar can you maybe run this through CMSSW testing?

@smuzaffar
Copy link
Contributor

smuzaffar commented Nov 18, 2024

I think this fix is relatively low-risk, but just to be sure: @aandvalenzuela @smuzaffar can you maybe run this through CMSSW testing?

cmssw tests are running via cms-sw#213

@github-actions
Copy link

github-actions bot commented Nov 18, 2024

Test Results

    17 files      17 suites   4d 2h 15m 16s ⏱️
 2 678 tests  2 677 ✅ 0 💤 1 ❌
43 752 runs  43 751 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit a14c665.

♻️ This comment has been updated with latest results.

@smuzaffar
Copy link
Contributor

@hahnjo , cmssw tests passed cms-sw#213 (comment)

@hahnjo
Copy link
Member Author

hahnjo commented Nov 19, 2024

@smuzaffar thanks for testing!

@hahnjo
Copy link
Member Author

hahnjo commented Nov 25, 2024

ping @vgvassilev

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Great work, @hahnjo! LGTM!

@hahnjo hahnjo merged commit 1e1f7ea into root-project:master Nov 27, 2024
@hahnjo hahnjo deleted the cling-autoload branch November 27, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants