[core] Reuse the CheckClassInfo lookup result in TClass::Init()

[guitargeek]

TClass::Init() first calls TCling::CheckClassInfo() (which looks up the class with cling's LookupHelper::findScope()) and then TCling::SetClassInfo(), which repeated the exact same lookup to construct the TClingClassInfo it stores in the TClass.

CheckClassInfo() now optionally returns the class info it found to the caller, and SetClassInfo() optionally accepts one, taking its ownership, instead of looking the class up again. TClass::Init() passes it through.

The result is only handed out when findScope() itself returned a declaration. findScope() only does so when the declaration points to an already complete definition (or a namespace), in which case its instantiateTemplate argument has no effect on the result; when the lookup only produced a forward-declared template specialization, findScope() returns a null declaration together with a non-null type, CheckClassInfo() hands nothing back, and SetClassInfo() performs the template-instantiating lookup exactly as before. The found type is passed along with the declaration so that typedef sugar (e.g. Double32_t) is conserved just like in the name-based lookup.

SetClassInfo() ignores a provided class info when 'reload' is set (a reload must redo the lookup) and in the tuple<...> special case, which overlays an alternate implementation with a different name.

The InsertStd() retry in CheckClassInfo() now collects the result type into a separate variable: findScope() does not write it on every path (e.g. when finding a namespace), so the retry could otherwise pair its declaration with a stale type left over from the first lookup.

This removes all duplicated findScope() calls during startup of root.exe: 206 -> 185 lookups, of which 9 were the TClingClassInfo constructor repeating a CheckClassInfo() lookup.

Fixes #7123

🤖 Done with the help of AI

[ferdymercury]

Thanks! Does this supersede #14760 I guess ?

[github-actions]

Test Results

    23 files      23 suites   3d 19h 3m 41s ⏱️
 3 865 tests  3 862 ✅ 0 💤 3 ❌
79 625 runs  79 621 ✅ 1 💤 3 ❌

For more details on these failures, see this check.

Results for commit a04ff4c.

[dpiparo]

Thanks! Does this supersede #14760 I guess ?

It's another stab at it for sure, building on top of the previous attempt.

[dpiparo]

Is it possible to also check how much memory this 10% reduction of the number of lookups entail?

[dpiparo]

I tried this very high level and and admittedly simplistic measurement

void Perf()
{
   ProcInfo_t info;
   gSystem->GetProcInfo(&info);
   printf(" usr  time = %f seconds\n",info.fCpuUser);
   printf(" sys  time = %f seconds\n",info.fCpuSys);
   printf(" res  memory = %g Mbytes\n",info.fMemResident/1024.);
   printf(" vir  memory = %g Mbytes\n",info.fMemVirtual/1024.);
}

and I did not see any effect of the PR, which is kind of surprising...

[guitargeek]

Hi @dpiparo, thanks for the check! Your results are expected. The second lookup was an exact repeat of one performed immediately before under the same interpreter lock, so the first lookup had already paid for any parsing and deserialization. The duplicate was a pure cache hit and allocated nothing lasting. So no memory reduction is expected from this PR, and no visible CPU time reduction either outside of synthetic microbenchmarks.

This PR fixes the architectural wart outlined in #7123: the duplicated lookups are redundant in principle and it is more elegant to avoid them. Reducing the number of calls here has no tangible performance or memory implications. It's a structural cleanup.

[guitargeek]

Thanks! Does this supersede #14760 I guess ?

Yes. This new PR achieves the same goal of fixing the issue, but it only reuses the result when the lookup returned a complete declaration, which avoids the template-instantiation semantics change and the lost typedef problem that made the CI fail in the #14760 attempt.

[dpiparo]

This PR fixes the architectural wart outlined in #7123: the duplicated lookups are redundant in principle and it is more elegant to avoid them. Reducing the number of calls here has no tangible performance or memory implications. It's a structural cleanup.

Thanks. And it's very useful, too, because it prepares the ground for a potential usage of the Interop technology to avoid string based look-ups altogether.