Additional fixes for std::pair's StreamerInfo handling by pcanal · Pull Request #10230 · root-project/root

pcanal · 2022-03-24T22:06:33Z

This repairs crash seem in testProxiesAndCategories on the v6.24 branches and a few additional issues seen during testing.

phsft-bot · 2022-03-24T22:06:40Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

pcanal · 2022-03-24T22:17:54Z

For the record the following issue is still left:

Issues with 'interpreted' + load-file-first mode.

Loading the file creates a TClass for the map.  Additional code (eg accessing
the CollectionProxy) might also create a TClass for the pair.

When the header file is loaded in the interpreter, unless there is explicit
uses of the map (or pair), there is no decl for the instantiation of thus
the TClass for the map and pair are not refreshed.

When the pair or map are instantiated (eg. `gInterpreter->Declare("pair<...> pl")`),
the TClass for the pair is informed (via `TCling::UpdateClassInfoWithDecl`
and `TCling::RefreshClassInfo`).

We could update `RefreshClassInfo` to refresh the `StreamerInfo` for the pair
but it would also need to also refresh the map's CollectionProxy (size,
hints, etc?) [and there is an arbitrary number because they are thread-local]

So at that point, it might actually be better to recreate the TClass for the
map ...

But wait ... there is currently no support for generating a collection proxy for
an interpreted class ... so it is actually an emulated collection proxy ...

That proxy does not match the interpreted (nor the compiled) version of the
map ... so there is no good point to match the pair either ....

So the solution above are (a tad bit) complex and .... not enough ...

phsft-bot · 2022-03-24T22:31:23Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-03-24T22:33:21Z

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

Failing tests:

phsft-bot · 2022-03-24T22:35:39Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-03-24T22:38:34Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-03-24T23:57:53Z

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

Failing tests:

phsft-bot · 2022-03-25T00:11:37Z

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

Failing tests:

phsft-bot · 2022-03-25T01:00:35Z

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

Failing tests:

phsft-bot · 2022-03-25T02:31:02Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2022-03-25T02:59:00Z

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

Failing tests:

And 7 more

phsft-bot · 2022-03-25T03:09:16Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 10 more

phsft-bot · 2022-03-25T03:55:52Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 1 more

phsft-bot · 2022-03-25T05:26:20Z

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

Failing tests:

And 7 more

phsft-bot · 2022-03-25T06:02:30Z

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

Failing tests:

And 2 more

phsft-bot · 2022-03-25T06:57:50Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 1 more

phsft-bot · 2022-03-25T07:16:51Z

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

Failing tests:

phsft-bot · 2022-03-30T00:20:26Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2022-03-30T00:46:31Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.evolution.roottest_root_meta_evolution_runforeign

phsft-bot · 2022-03-30T00:47:59Z

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

Failing tests:

projectroot.roottest.root.meta.evolution.roottest_root_meta_evolution_runforeign

phsft-bot · 2022-03-30T00:56:23Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.evolution.roottest_root_meta_evolution_runforeign

phsft-bot · 2022-03-30T00:59:41Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

This is done also for multimap.

When encountering a data member of a class that is a enum or if it is use in a map or multimap, record the enum in the rootpcm.

This was a fatal typo in 9678405 pair: properly update StreamerInfo.

We already had TListOfEnumsWithLock but TListOfEnums::FindObject is used by rootcling

In the 'root_serialization' use case, we ended in TBufferFile::WriteClassBuffer with no StreamerInfo created yet for the TClass for `std::vector<float>` and thus go into the branch that creates the StreamerInfo ... However, at least in that case, the call to `BuildRealData` induced the creation of the StreamerInfo but the code in `TBufferFile::WriteClassBuffer` did not notice and thus create a second one and tried to register it in the same slot. (The actual negative side effect was only an error message and a memory leak).

This happened in a user case where the container (map) for a pair of an enum, which had not dictionary and something else that did have a dictionary.

This was happening when, inadvertently, StreamerInfo were created multiple times and then immeditately deleted (for map<enumtype_without_dict, somethinelse>)

phsft-bot · 2022-04-01T16:16:05Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

Axel-Naumann

Lgtm more or less :-)

Axel-Naumann · 2022-04-06T07:53:50Z

core/cont/inc/TVirtualCollectionProxy.h

   virtual ~TVirtualCollectionProxy() {};

+   // Reset the info gathered from StreamerInfos and value's TClass.
+   virtual Bool_t    Reset() { return kTRUE; };


Suggested change

virtual Bool_t Reset() { return kTRUE; };

virtual Bool_t Reset() { return kTRUE; }

What does the return value indicate?

That the reset failing somehow ... I guess this could/should be replaced by throwing an exception but this code is not using them anywhere else ....

Axel-Naumann · 2022-04-06T07:59:35Z

core/meta/src/TClass.cxx

+
+            fCollectionProxy->Reset();
+            TIter nextClass(gROOT->GetListOfClasses());
+            while (auto acl = (TClass*)nextClass()) {


Really?! This can iterate through a million classes with the experiments...

Well, only, 10s of thousands but yep ... However the case should be rare-ish this days. It requires: "load file, read data and/or manipulate the TClass and then load the library" (Note that we have a similar loop in TClass::ReplaceWith that so far as not been 'noticed').

Axel-Naumann · 2022-04-06T08:03:06Z

core/meta/src/TClass.cxx

+            TIter nextClass(gROOT->GetListOfClasses());
+            while (auto acl = (TClass*)nextClass()) {
+               if (acl == this) continue;
+               if (acl->fCollectionProxy && acl->fCollectionProxy->GetValueClass() == pcl) {


Alternatively you could flag the state of the collection proxy as "before we have authoritative info", and then reset and update in GetValueClass() when pcl's state doesn't correspond to that flag?

I don't think it would change much. It is already functionally the case (the GetValueClass() == pcl). For fully initialized proxy this is "just" a function call and an atomic pointer load (the alternative would only, possibly, save the function call by inlining).

Axel-Naumann · 2022-04-06T09:09:53Z

core/meta/src/TClass.cxx

+               }
+            }
+
+            TIter next(pcl->GetStreamerInfos());


Would it not make sense to update this before telling all collection proxies to update their value class, just in case in the future we get a dependency of the proxies on the value steamer info?

Possibly, this is a 'high' risk change (potential order of deletions and/or state change issues, etc.). So I will try it in a different PR (and there is a potential optimization with the same type of risk 5 lines above that I'll try too)

Actually it is confirmed. The objects (ActionSequence) deleted in the CollectionProxy's Reset are pointing/using the information from the StreamerInfo (that is deleted/destroyed by the StreamerInfo's Clear).

Axel-Naumann · 2022-04-06T09:11:53Z

core/meta/src/TListOfEnums.cxx

+/// Specialize FindObject to do search for the
+/// a enum just by name or create it if its not already in the list
+
+TObject *TListOfEnums::FindObject(const char *name) const


Do other overloads of FindObject also add? I don't remember that... Maybe consider renaming to FindOrAdd()?

They should :) ... at least for this (type) of container. Semantically 'nothing' is added, it is just delayed loading. This is a trade-off between pre-loading all enums (and actively harvesting them at each transaction) and this type of gymnastics. (i.e. the cost of avoiding this const_cast is added unused memory alllocation/retention and added (unused) run-time cost)

core/meta/src/TListOfEnums.cxx

Axel-Naumann · 2022-04-06T09:14:35Z

core/meta/src/TListOfEnums.cxx

+      TInterpreter::DeclId_t decl;
+      if (GetClass()) decl = gInterpreter->GetEnum(GetClass(), name);
+      else            decl = gInterpreter->GetEnum(nullptr, name);
+      if (decl) result = const_cast<TListOfEnums *>(this)->Get(decl, name);


Eew. Can we make this function non-const?

Not without being backward incompatible (We would probably need to expose this type rather than its base). Note: this is existing code that is just moved around.

Axel-Naumann · 2022-04-06T09:15:53Z

core/meta/src/TProtoClass.cxx

   {
-      auto temp = new TListOfEnums();
+      auto temp = cl->fEnums.load() ? cl->fEnums.load() :
+                  IsFromRootCling() ? new TListOfEnums() : new TListOfEnumsWithLock();


Is the lock so expensive to warrant the extra code complexity?

Probably not, especially since the mutex are probably not even on in the IsFromRootCling. This one liners is the easiest fix and so we should remove this complexity in a different PR. See #10339 for tracking.

phsft-bot · 2022-04-06T12:34:36Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

pcanal requested a review from Axel-Naumann as a code owner March 24, 2022 22:06

pcanal self-assigned this Mar 24, 2022

pcanal added priority:critical in:Core Libraries in:I/O labels Mar 24, 2022

pcanal requested a review from jblomer March 24, 2022 22:07

pcanal marked this pull request as draft March 25, 2022 03:35

pcanal force-pushed the master-issue-10131 branch from bfc3736 to 8c249ba Compare March 30, 2022 00:20

pcanal added 16 commits April 1, 2022 11:14

When loading a TClass for map, also update the pair's TClass.

8b6224d

This is done also for multimap.

TStreamerInfo::Clear, also reset TStreamerElement's offset

87ca01d

rootcling: record Enum used by classes and collection.

f3b9889

When encountering a data member of a class that is a enum or if it is use in a map or multimap, record the enum in the rootpcm.

io: Correct offset passed from SyntheticPair to StreamerInfo.

e765b5d

This was a fatal typo in 9678405 pair: properly update StreamerInfo.

Add missing TListOfEnums::FindObject.

72fb9b2

We already had TListOfEnumsWithLock but TListOfEnums::FindObject is used by rootcling

TGenCollectionProxy: add missing nullptr protection

a43074b

When resetting a pair's StreamerInfo inform all collection proxies

42fa5cf

TProtoClass::FillTClass: Create a ListEnumsWithLock when needed

677b676

Don't put class' enum in rootpcm global list of enums

87c4b0c

pair: Also treat unordered map

fcd49be

TClass: clear fLastReadInfo, fCurrentInfo when deleting TStreamerInfos

82e89ab

Do the pair TClass StreamerInfo refresh only when needed

2b5863f

TGenCollectionProxy init: fail if the value's StreamerInfo is not there.

1bcc0f1

This happened in a user case where the container (map) for a pair of an enum, which had not dictionary and something else that did have a dictionary.

Avoid reading pass the end of the StreamerInfo container.

334a998

This was happening when, inadvertently, StreamerInfo were created multiple times and then immeditately deleted (for map<enumtype_without_dict, somethinelse>)

CollectionProxy: pass pair hint to R__CreateValue

6189bb9

pcanal force-pushed the master-issue-10131 branch from a16f3c8 to 6189bb9 Compare April 1, 2022 16:15

guitargeek mentioned this pull request Apr 5, 2022

[RF] New test reproducing RooWorkspace crash after TStreamerInfo update root-project/roottest#863

Merged

pcanal linked an issue Apr 5, 2022 that may be closed by this pull request

[RF] Crash in reading some RooWorkspaces after recent TStreamerInfo update #10282

Closed

This was referenced Apr 6, 2022

v626: Additional fixes for std::pair's StreamerInfo handling #10334

Closed

v626: Additional fixes for std::pair's StreamerInfo handling #10335

Merged

Axel-Naumann approved these changes Apr 6, 2022

View reviewed changes

pcanal added 2 commits April 6, 2022 07:32

Remove unnecessary trailing semi-colon

e3bac5a

[NFC] Simplify code in TListOfEnums

a0dd6b0

pcanal merged commit cd90139 into root-project:master Apr 6, 2022

pcanal deleted the master-issue-10131 branch April 6, 2022 15:50

pcanal linked an issue Apr 6, 2022 that may be closed by this pull request

Open too many different non-versioned layouts for pair #10131

Closed

pcanal mentioned this pull request Apr 25, 2022

Warnings about Missing std::pair dictionaries with 6.24 #7960

Closed

	virtual Bool_t Reset() { return kTRUE; };
	virtual Bool_t Reset() { return kTRUE; }

Conversation

pcanal commented Mar 24, 2022

Uh oh!

phsft-bot commented Mar 24, 2022

Uh oh!

pcanal commented Mar 24, 2022

Uh oh!

phsft-bot commented Mar 24, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 24, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 24, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 24, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 24, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 25, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 30, 2022

Uh oh!

phsft-bot commented Mar 30, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 30, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 30, 2022

Failing tests:

Uh oh!

phsft-bot commented Mar 30, 2022

Failing tests:

Uh oh!

phsft-bot commented Apr 1, 2022

Uh oh!

Axel-Naumann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!