-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Timing Service now measures time not running a Module #41262
Conversation
Previously the signal happened after all Records were asynchronously updated to their new IOVs. Now the signals focus on the timings of calls to the ESSources.
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41262/35015
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41262/35016
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: ClangBuild Clang BuildI found compilation warning while trying to compile with clang. Command used:
See details on the summary page. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41262/35104
|
Pull request #41262 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d35193/31879/summary.html Comparison SummarySummary:
|
@Dr15Jones Could you add the other changes of the PR (the moment where "init" accounting changes to "loop" accounting, change in where |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
will this PR allow counting the time spent on GPU as well? IIUC, currently it's not included in the TimingService per-module totals. |
No. Already the exact meaning of "time spent on GPU" is complex enough to warrant its own issue and discussion (although #29142 is somewhat related). |
+1 |
what about at least in a single thread/stream execution case? |
I can imagine several possible things to measure as "time spent on GPU" even then, so I'd rather figure out first what quantity/quantities exactly make sense to measure (suggestions are welcome in a separate issue), and then craft a general solution for that. But I acknowledge that a long running "external worker" is indeed one source of "non-module time" in a situation where the framework('s thread) has nothing else to do than wait for the work to complete. |
PR description:
The built in timing service now calculates the amount of time all cores are not running a module (i.e. Source, EDProducer, EDFilter, EDAnalyzer, OutputModule, ESModule or ESSource) during the data processing loop. This gets reported in the summary and to the framework job report. Time is summed across all threads.
Additional changes:
PR validation:
Framework unit tests pass.