[core] Add a stub for proper re-reading of rdicts if the library is reloaded. #5420
Conversation
|
Starting build on |
| " ~DictInit() {\n" | ||
| " assert (isInitialized);" | ||
| " TROOT::UnRegisterModule(\"" << demangledDictName << "\"," | ||
| "TriggerDictionaryInitialization_" << dictName << "_Impl);\n" |
There was a problem hiding this comment.
I suppose that this is misleading (albeit harmeless) as you would a want a UnTrigger function here.
There was a problem hiding this comment.
I use that function to find back the shared object. We can avoid that by keeping a mapping in TCling between modulename and shared object. Then this function will take only one argument.
There was a problem hiding this comment.
Shall I make that change?
There was a problem hiding this comment.
I would prefer that (but feel free not too if you prefer to leave it as is).
There was a problem hiding this comment.
@pcanal, could you provide an implementation of TCling::UnLoadPCM?
|
@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On |
|
Starting build on |
|
Build failed on mac1015/cxx17. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. |
|
Build failed on ROOT-fedora29/python3. |
When a library is re-loaded (via .L) we should reload the corresponding rdict file. However current ROOT (without modules) just re-reads the rdict file. This patch brings this behavior for modules and is intended to be a quick fix for the os nightly failures of kind: Error in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcmError in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcm The continuation of this work is in root-project#5420
Do we still distinguish (as I think we ought to) between the case .L which might reload the library (probably 'only' if it changed) and the case gSystem->Load which should not unload the library (and for that matter I would also not really expect |
When a library is re-loaded (via .L) we should reload the corresponding rdict file. However current ROOT (without modules) just re-reads the rdict file. This patch brings this behavior for modules and is intended to be a quick fix for the os nightly failures of kind: Error in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcmError in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcm The continuation of this work is in #5420
Can you remind me why: |
|
On the jenkins build node the error appears likely because of '-Dsoversion="On"' and the following. One test that fails is 'runload.C' in roottest/root/treeformula/sync. There we have (essentially): The use of TTree means that before executing the script libTree is laoded. which coupled with the new feature that gSystem->Load first dlclose the library then reopen it means that libTree is dlclose 2 or 3 times (one for both the versioned and unversioned version). This behavior is a very significant departure from the existing behavior (where, because the libraries were added to the link line, the library were never reloaded). Reloading arbitrary depend library is never a good idea since they (are likely to) include static object that may do things that are essential to do only once (initialization, connection to db). Technically CompileMacro could (as it does elsewhere) first check if the library is loaded. But still several questions:
@Axel-Naumann @vgvassilev opinions? PS. Even-though the set of test would be fixed by updating gSystem->Load and/or CompileMacro, the PR is still likely necessary for the case where CompileMacro generates a pcm (and thus need to support reload) or the case where the user explicitly unload a library. |
vgvassilev
force-pushed
the
fix_osx_reload_rdicts
branch
from
03acedc to
afe3d65
Compare
|
Starting build on |
|
Build failed on mac1015/cxx17. Errors:
|
Because ROOT need to be built with |
The libTree.6.21.01.so libTree.so are the same library -- one is linked to the other (forgot which way it was). The dependent symbol scanner currently does not work well with symlinks. This PR fixes it: #4717
If we decide that it won't ever reload (which I do not see issues with) then we should reimplement the calls to
|
|
As |
And it would not (and should not) matter except that it currently leads to the library being reloaded yet an additional time (for every macro that is loaded and uses TTree!!!)
I think TSystem::Load should behave the same as dlopen (once upon a time it was a thin layer around it).
There a case to be made for returning TSystem::Load to never reload while keeping gInterpreter->Load and ".L" being the same (so TSystem::Load and gInterpreter->Load behaving differently). In my opinion ".L ...so" (and thus gInterpreter->Load) have several possibilities but they are indeed intimately link to the behavior of ".L ....C" and ".L ...C+", so exhaustive list: (1.a) never reload *.so, never reload *.C and *.C+ (2.a) only reload *.so when changed, never reload *.C and *.C+ (3.a) always reload *.so, never reload *.C and *.C+ Note: CompileMacro was implemented with the "only reload *.C+ when changed in mind). (2.b) has the minimal amount of unloading but consequently has an 'unpredictable' number of static initialization. So (1.b) might be a better alternative if we assume that user macro are unlikely to have static initialization that matter/must-be-exact (and if it does then the user can use explicit .U I guess). Because ".L .so" might be used to load 'global' (for example Athena's or CMSSW's) libraries, I find (3.) too harsh. |
vgvassilev
force-pushed
the
fix_osx_reload_rdicts
branch
from
afe3d65 to
17d667d
Compare
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on ROOT-fedora29/python3. Failing tests: |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
|
||
| void TCling::UnLoadPCMImpl(const TFile& rdict) | ||
| { | ||
| // FIXME: Reverse the effect of LoadPCMImpl. |
There was a problem hiding this comment.
@pcanal, I have put all things in place except for this ;)
There was a problem hiding this comment.
@pcanal, the fate of this PR depends on this.
There was a problem hiding this comment.
@pcanal, I see you were working in a similar area, maybe you can also look into this and we can land this PR.
|
@phsft-bot build! |
|
Starting build on |
|
Build failed on ROOT-fedora31/noimt. Failing tests:
|
|
Build failed on mac1015/cxx17. Failing tests: |
|
Build failed on ROOT-performance-centos7-multicore/default. Failing tests:
|
|
Build failed on ROOT-fedora30/cxx14. Failing tests: |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
Build failed on ROOT-fedora29/python3. Failing tests: |
|
Converted to draft because "work in progress" label will be discarded. |
When a library is re-loaded (via .L) we should reload the corresponding rdict file. However current ROOT (without modules) just re-reads the rdict file. This patch brings this behavior for modules and is intended to be a quick fix for the os nightly failures of kind: Error in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcmError in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcm The continuation of this work is in root-project#5420
|
@pcanal, ping. |
…oaded. If we call TCling::Load on an already loaded library we dlclose and dlopen the library. However, currently we do not have a facility to 'reload'/undo the effect of TCling::LoadPCM. This patch adds a section in the dictionary for de-registration which is reverse to TCling::RegisterModule -- TCling::UnRegisterModule. It tracks down which library is being reloaded and does not trigger a re-read of the rdict pcm. The current stub is can be further expanded to undo the effects caused by TCling::RegisterModule and/or improve the TCling shutdown by running the interpreter-dependent shutdown of the dictionary.
vgvassilev
force-pushed
the
fix_osx_reload_rdicts
branch
from
17d667d to
a21a9ef
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests:
And 57 more |
|
Build failed on windows10/cxx14. |
|
Build failed on mac1014/python3. Failing tests: |
|
Build failed on mac11.0/cxx17. Failing tests:
|
6 participants
If we call TCling::Load on an already loaded library we dlclose and dlopen the library. However, currently we do not have a facility to 'reload'/undo the effect of TCling::LoadPCM.
This patch adds a section in the dictionary for de-registration which is reverse to TCling::RegisterModule -- TCling::UnRegisterModule. It tracks down which library is being reloaded and does not trigger a re-read of the rdict pcm. The current stub is can be further expanded to undo the effects caused by TCling::RegisterModule and/or improve the TCling shutdown by running the interpreter-dependent shutdown of the dictionary.
The intent of this patch is to fix the failing OSX tests with
Error in TCling::LoadPCM: ROOT PCM /.../libTree_rdict.pcm file does not exist
Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImageGui_rdict.pcm
Info in TCling::LoadPCM: In-memory ROOT PCM candidate /.../libASImage_rdict.pcm