Skip to content

Conversation

@vgvassilev
Copy link
Member

@vgvassilev vgvassilev commented Dec 28, 2019

This patch consolidates the symbol resolution facilities throughout TCling into
a new singleton class Dyld part of the cling's DynamicLibraryManager.

The new dyld is responsible for:

  • Symlink resolution -- it implements a memory efficient representation of
    the full path to shared objects allowing search at constant time O(1). This
    also fixes issues when resolving symbols from OSX where the system libraries
    contain multiple levels of symlinks.
  • Bloom filter optimization -- it uses a stohastic data structure which gives
    a definitive answer if a symbol is not in the set. The implementation checks
    the .gnu.hash section in ELF which is the GNU implementation of a bloom
    filter and uses it. If the symbol is not in the bloom filter, the
    implementation builds its own and uses it. The measured performance of the
    bloom filter is 30% speed up for 2mb more memory. The custom bloom filter on
    top of the .gnu.hash filter gives 1-2% better performance.
    The advantage for the custom bloom filter is that it works on all
    implementations which do not support .gnu.hash (windows and osx). It is also
    customizable if we want to further reduce the false positive rates
    (currently at p=2%).
  • Hash table optimization -- we build a hash table which contains all symbols
    for a given library. This allows us to avoid the fallback symbol iteration
    if multiple symbols from the same library are requested. The hash table
    optimization targets to optimize the case where the bloom filter tells us
    the symbol is maybe in the library.

Patch by Alexander Penev (@alexander-penev) and me!

Performance Report

platform test PCH-time Module-time Module-PR-time
osx 10.14 roottest-python-pythonizations 22,82 26,89 20,08
osx 10.14 roottest-cling 589,67 452,97 307,34
osx 10.14 roottest-python 377,69 475,78 311,5
osx 10.14 roottest-root-hist 60,59 90,98 49,65
osx 10.14 roottest-root-math 106,18 140,41 73,96
osx 10.14 roottest-root-tree 1287,53 1861 1149,35
osx 10.14 roottest-root-treeformula 568,43 907,46 531
osx 10.15 root-io-stdarray - 126.02 31.42
osx 10.15 roottest-root-treeformula - 327.08 231.14

The effect of running ctest -j8:

platform test PCH-time Module-time Module-PR-time
osx 10.14 roottest-python-pythonizations 14,45 18,89 13,03
osx 10.14 roottest-cling 88,96 118,94 100,1
osx 10.14 roottest-python 107,57 60,93 100,88
osx 10.14 roottest-root-hist 10,25 23,25 11,77
osx 10.14 roottest-root-math 8,33 21,23 9,27
osx 10.14 roottest-root-tree 555 840,89 510,97
osx 10.14 roottest-root-treeformula 235,44 402,82 228,91

We think in -j8 we lose the advantage of the new PR because the PCH had the rootmaps read in memory and restarting the processes allowed the kernel efficiently reuse that memory. Whereas, the modules and this PR scans the libraries from disk and builds in-memory optimization data structures. Reading from disk seems to be the bottleneck (not verified) but if that's an issue in future we can write out the index making subsequent runs at almost zero cost.

@vgvassilev vgvassilev requested a review from pcanal as a code owner December 28, 2019 08:10
@vgvassilev vgvassilev self-assigned this Dec 28, 2019
@vgvassilev vgvassilev force-pushed the cxxmodules-modules-bloomfilter branch from fa0e485 to ff3b307 Compare December 29, 2019 12:11
@vgvassilev
Copy link
Member Author

@phsft-bot build!

@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on mac1014/cxx17.
See console output.

Failing tests:

@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on mac1014/cxx17.
See console output.

Warnings:

  • /ec/build/workspace/root-pullrequests-build/root/core/metacling/src/TClingDyld.h:157:31: warning: 'RTTIContextCall_ResolveSymbol' has C-linkage specified, but returns user-defined type 'std::string' (aka 'basic_string<char, char_traits<char>, allocator<char> >') which is incompatible with C [-Wreturn-type-c-linkage]
  • /ec/build/workspace/root-pullrequests-build/root/core/metacling/src/TClingDyld.h:157:31: warning: 'RTTIContextCall_ResolveSymbol' has C-linkage specified, but returns user-defined type 'std::string' (aka 'basic_string<char, char_traits<char>, allocator<char> >') which is incompatible with C [-Wreturn-type-c-linkage]
  • /ec/build/workspace/root-pullrequests-build/root/core/metacling/src/TClingDyld.cxx:107:1: warning: 'LibraryPath' defined as a struct here but previously declared as a class; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]

@vgvassilev vgvassilev force-pushed the cxxmodules-modules-bloomfilter branch from a8fd3c8 to 337dc6c Compare December 30, 2019 19:17
@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14
How to customize builds

@vgvassilev
Copy link
Member Author

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@vgvassilev vgvassilev changed the title [WIP] [tcling] Improve symbol resolution. [tcling] Improve symbol resolution. Dec 31, 2019
@vgvassilev
Copy link
Member Author

@phsft-bot build just on mac1014 with flags -Druntime_cxxmodules=On

@vgvassilev
Copy link
Member Author

@phsft-bot build just on mac1014/cxx17 with flags -Druntime_cxxmodules=On

@phsft-bot
Copy link

Starting build on mac1014/cxx17 with flags -Druntime_cxxmodules=On
How to customize builds

@phsft-bot
Copy link

Build failed on mac1014/cxx17.
See console output.

@vgvassilev vgvassilev force-pushed the cxxmodules-modules-bloomfilter branch from 337dc6c to 35f29cd Compare January 2, 2020 10:18
@phsft-bot
Copy link

Starting build on mac1014/cxx17 with flags -Druntime_cxxmodules=On
How to customize builds

@phsft-bot
Copy link

Starting build on mac1014/cxx17 with flags -Druntime_cxxmodules=On
How to customize builds

@phsft-bot
Copy link

Starting build on mac1014/cxx17 with flags -Druntime_cxxmodules=On
How to customize builds

@phsft-bot
Copy link

Build failed on mac1014/cxx17.
See console output.

Errors:

  • clang: error: linker command failed with exit code 1 (use -v to see invocation)

@vgvassilev vgvassilev force-pushed the cxxmodules-modules-bloomfilter branch from 13a7e7e to b9f5014 Compare January 4, 2020 20:54
@phsft-bot
Copy link

Starting build on mac1014/cxx17 with flags -Druntime_cxxmodules=On
How to customize builds

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

This patch consolidates the symbol resolution facilities throughout TCling into
a new singleton class Dyld part of the cling's DynamicLibraryManager.

The new dyld is responsible for:
  * Symlink resolution -- it implements a memory efficient representation of
    the full path to shared objects allowing search at constant time O(1). This
    also fixes issues when resolving symbols from OSX where the system libraries
    contain multiple levels of symlinks.
  * Bloom filter optimization -- it uses a stohastic data structure which gives
    a definitive answer if a symbol is not in the set. The implementation checks
    the .gnu.hash section in ELF which is the GNU implementation of a bloom
    filter and uses it. If the symbol is not in the bloom filter, the
    implementation builds its own and uses it. The measured performance of the
    bloom filter is 30% speed up for 2mb more memory. The custom bloom filter on
    top of the .gnu.hash filter gives 1-2% better performance.
    The advantage for the custom bloom filter is that it works on all
    implementations which do not support .gnu.hash (windows and osx). It is also
    customizable if we want to further reduce the false positive rates
    (currently at p=2%).
  * Hash table optimization -- we build a hash table which contains all symbols
    for a given library. This allows us to avoid the fallback symbol iteration
    if multiple symbols from the same library are requested. The hash table
    optimization targets to optimize the case where the bloom filter tells us
    the symbol is *maybe* in the library.

Patch by Alexander Penev and me!
@vgvassilev vgvassilev force-pushed the cxxmodules-modules-bloomfilter branch from ca88c9b to ee856ff Compare May 6, 2020 14:48
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-performance-centos7-multicore/default.
Running on olhswep22.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See cdash.
See console output.

Errors:

  • [2020-05-06T15:23:43.260Z] 636/2022 Test [TDF] minor fixes #370: tutorial-dataframe-df103_NanoAODHiggsAnalysis .....................................................***Failed Error regular expression found in output. Regex=[: error:] 4.22 sec

Failing tests:

@vgvassilev
Copy link
Member Author

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-fedora29/python3.
Running on root-fedora29-3.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on mac1015/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

Copy link
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

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

Please revisit the open points, if needed in a new PR

@vgvassilev vgvassilev merged commit 736aaa2 into root-project:master May 6, 2020
@vgvassilev vgvassilev deleted the cxxmodules-modules-bloomfilter branch May 6, 2020 20:03
vgvassilev added a commit to vgvassilev/root that referenced this pull request May 7, 2020
@vgvassilev vgvassilev mentioned this pull request May 7, 2020
vgvassilev added a commit that referenced this pull request May 7, 2020
This is part of #4717
FonsRademakers pushed a commit to root-project/cling that referenced this pull request May 7, 2020
vgvassilev added a commit to vgvassilev/root that referenced this pull request May 7, 2020
vgvassilev added a commit that referenced this pull request May 7, 2020
FonsRademakers pushed a commit to root-project/cling that referenced this pull request May 7, 2020
vgvassilev added a commit to vgvassilev/root that referenced this pull request May 18, 2020
This addresses the rest of the comments of root-project#4717

Patch by Alexander Penev and me.
vgvassilev added a commit that referenced this pull request May 25, 2020
This addresses the rest of the comments of #4717

Patch by Alexander Penev and me.
FonsRademakers pushed a commit to root-project/cling that referenced this pull request May 25, 2020
This addresses the rest of the comments of root-project/root#4717

Patch by Alexander Penev and me.
mrodozov pushed a commit to cms-sw/root that referenced this pull request Jun 4, 2020
This addresses the rest of the comments of root-project#4717

Patch by Alexander Penev and me.
osschar pushed a commit to osschar/root that referenced this pull request Dec 21, 2020
osschar pushed a commit to osschar/root that referenced this pull request Dec 21, 2020
osschar pushed a commit to osschar/root that referenced this pull request Dec 21, 2020
This addresses the rest of the comments of root-project#4717

Patch by Alexander Penev and me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants