Remove use of thread_local from ModuleAllocMonitor

[Dr15Jones]

PR description:

Problems were seen related to thread local algorithm being called recursively. Replaced thread local with static allocation assignment using thread ids.

PR validation:

Tested using step1 of workflow 11834.21 which previously had a segmentation fault when used with ModuleAllocMonitor and multiple threads. Now it worked.

fixes #45964

[Dr15Jones]

please test

[cmsbuild]

cms-bot internal usage

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46606/42504

[cmsbuild]

A new Pull Request was created by @Dr15Jones for master.

It involves the following packages:

• PerfTools/AllocMonitor (core)

@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@felicepantaleo, @makortel this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[cmsbuild]

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-991153/42605/summary.html
COMMIT: 11ad172
CMSSW: CMSSW_14_2_X_2024-11-05-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46606/42605/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially removed 1 lines from the logs
• Reco comparison results: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 46
• DQMHistoTests: Total histograms compared: 3343138
• DQMHistoTests: Total failures: 423
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3342695
• DQMHistoTests: Total skipped: 20
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
• Checked 195 log files, 172 edm output root files, 46 DQM output files
• TriggerResults: no differences found

[makortel]

I see the approach is similar to #45150, but not exactly the same. Why does ModuleAllocMonitor need more complex ThreadTracker? (are e.g. collisions more probable?)

I also wonder if it would feasible to share some of these complicated codes between here and memory_proxies.cc? (although I wouldn't be surprised our requirements for the separation of these components could prohibit straightforward ways of using common code)

[Dr15Jones]

The AllocMonitorPreload uses a lock to guarantee that if there is a hash collision, only one thread at a time can be in the critical section (i.e. stop reporting of allocation/deallocation requests on that thread). The lock is expected to be held only for a tiny amount of time and no subsequent need for data after leaving that critical section. Therefore threads are allowed to share the same index, it just makes the job a bit slower if they do because of the possible lock contention.

In this PR, each thread must have its own info for a long period of time (i.e. the time period for a module processing an event). If I were to use a mutex to resolve any possible hash collisions of the thread ids, then we could potentially slow down the job a great deal as threads would have to wait a long time. Also, if for some reason an end service signal did not fire so the lock was never released the job would probably deadlock. Therefore assigning permanent indices into the allocation info containers for each thread id seemed to be the potentially faster and safer option. In this code the first event to hash into the container gets the 'O(1)' access and any subsequent threads that have the same hash get relegated to the linear lookup access.

[makortel]

Could you also add a note in the README.md that the AllocMonitor implementations should not use thread_local, maybe with a pointer to ModuleAllocMonitor if per-thread storage is needed? (although if that happens I'd like to consider stronger a shared implementation)

[makortel]

Why these free "hashed threads" are marked differently from the "extra threads"?

[makortel]

Does this special value for a free "hashed thread" mean there is an assumption the thread ID can not be smaller than kHashedEntries? (probably it is a good assumption, but nevertheless)

[Dr15Jones]

For the hashed thread all the thread ids have been modulo-ed by kHashedEntries. The value initially assigned to the hashed_threads_ entry is actually a value that is impossible to be allowed in that spot with such a modulo (i.e. it is always 1 larger than the smallest allowed value in that spot).

Given the extra thread is linearly search, there is notvalue that can be safely inserted as the initial value for an entry. I assumed that no thread would be assigned ~entry_type(0).

[makortel]

Thanks, got it. Could you add these comments to code itself (for the next reader)?

[Dr15Jones]

I updated the comments.

[Dr15Jones]

README.md was updated with a warning about thread_local and I added an entry for ModuleAllocMonitor which was missing.

[Dr15Jones]

please test

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46606/42535

[cmsbuild]

Pull request #46606 was updated. @Dr15Jones, @makortel, @smuzaffar can you please check and sign again.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46606/42536

[cmsbuild]

Pull request #46606 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46606/42537

[Dr15Jones]

please test

[cmsbuild]

Pull request #46606 was updated. @Dr15Jones, @makortel, @smuzaffar can you please check and sign again.