Cleaning up subProcesses looping. #16292

knoepfel · 2016-10-20T21:23:01Z

First commit is only spacing. Second commit removes need for calling hasSubProcesses multiple times.

- Changed subProcesses_ to be a vector<SubProcess> object, instead of smart pointer to same. If empty, looping over the object is a nop. - Change return type of popVParameterSet() to vector<ParameterSet> instead of unique_ptr to same. I don't believe this introduced any breaking changes, outside of FWCore. Conflicts: FWCore/Framework/interface/SubProcess.h FWCore/Framework/src/EventProcessor.cc FWCore/Framework/src/SubProcess.cc FWCore/ParameterSet/src/ProcessDesc.cc

cmsbuild · 2016-10-20T21:23:19Z

A new Pull Request was created by @knoepfel for CMSSW_8_1_X.

It involves the following packages:

FWCore/Framework
FWCore/ParameterSet
FWCore/ServiceRegistry

@cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @wddgit, @wmtan this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

Dr15Jones · 2016-10-20T21:30:31Z

FWCore/Framework/src/EventProcessor.cc

-                                                std::make_pair(itPS->getUntrackedParameter<std::string>("type", "*"),
-                                                               itPS->getUntrackedParameter<std::string>("label", "")));
+    for(auto const& ps : excluded) {
+      eventSetupDataToExcludeFromPrefetching_[ps.getUntrackedParameter<std::string>("record")].insert(


emplace would avoid the need to create a std::pair.

Yep. I limited most of my changes to subProcesses_, but this one could easily go in, too.

Dr15Jones · 2016-10-20T21:33:17Z

FWCore/Framework/src/EventProcessor.cc

@@ -486,13 +483,13 @@ namespace edm {
    ScheduleItems items;

    //initialize the services
-    std::shared_ptr<std::vector<ParameterSet> > pServiceSets = processDesc->getServicesPSets();
-    ServiceToken token = items.initServices(*pServiceSets, *parameterSet, iToken, iLegacy, true);
+    auto pServiceSets = processDesc->getServicesPSets();


I believe the p in pServiceSets stood for pointer. Now it could just be serviceSets.

This is also forcing a copy. Is the copy needed? if not then it should be changed to auto const&.

Can change pServiceSets to serviceSets. There's no copy though...it's a move-construction of pServiceSets, which is slightly less efficient than auto const&.

I do not believe you are correct. The signature for the member function is

auto const& getServicesPSets() const {return services_;}

Therefore it can not do a move construction and must do a copy.

Ack! Good catch. Will change.

Turns out it needed to be auto&, since initServices requires an lvalue reference.

Dr15Jones · 2016-10-20T21:50:32Z

FWCore/Framework/src/SubProcess.cc

-    bool hasSubProcesses = subProcessVParameterSet.get() != nullptr;
-
+    auto subProcessVParameterSet = popSubProcessVParameterSet(*processParameterSet_);
+    bool hasSubProcesses = subProcessVParameterSet.size() != 0ull;


not subProcessVParameterSet.empty() might be clearer.

Dr15Jones · 2016-10-20T22:00:53Z

FWCore/ParameterSet/src/ProcessDesc.cc

      std::string name = it->getParameter<std::string>("@service_type");
      if (name == service) {
        // Use the configured service.  Don't add a default.
        // However, the service needs to be moved to the front because it is a standard service.
        ParameterSet pset = *it;
-        services_->erase(it);
+        services_.erase(it);


Does erasing invalidate the iterator? If so, then it isn't safe to do while using the iterator to loop.

Well, the erase was in there before I tweaked it. And yes, in general, it does invalidate the iterator. But in this case, and the following, the loop is exited immediately after the erase (either by return or break); it's therefore safe in these cases.

Your right. I guess it's best not to try to review code just before your trying to go home :).

Dr15Jones · 2016-10-20T22:04:00Z

please test

cmsbuild · 2016-10-20T22:04:13Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15867/console
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15868/console

Dr15Jones · 2016-10-20T22:05:15Z

@davidlange6 although this shouldn't have any functional changes, it would be fine to have this wait until CMSSW_9_0 opens if that would make your life easier.

cmsbuild · 2016-10-21T01:06:26Z

-1

Tested at: d29f764

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16292/15868/summary.html

I found follow errors while testing this PR

Failed tests: Build

Build:

I found an error when building:

                 from /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/src/TMVAEvaluator.cc:1:
/cvmfs/cms-ib.cern.ch/week1/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_8_1_X_2016-10-20-2300/src/CondFormats/EgammaObjects/interface/GBRForest.h:1:1: warning: null character(s) ignored
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/src/ExpressionFunctionSetter.cc:6:35: warning: /cvmfs/cms-ib.cern.ch/2016-43/slc6_amd64_gcc530/lcg/root/6.06.08-giojec/include/Math/ProbFuncMathCore.h is shorter than expected
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/src/findMethod.cc 
In file included from /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/src/TMVAEvaluator.cc:1:0:
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/interface/TMVAEvaluator.h:39:41: error: missing binary operator before token "("
     #if ROOT_VERSION_CODE < ROOT_VERSION(6,7,0)
                                         ^
In file included from /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/interface/TMVAZipReader.h:26:0,
                 from /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-10-20-2300/src/CommonTools/Utils/src/TMVAEvaluator.cc:3:
/cvmfs/cms-ib.cern.ch/2016-43/slc6_amd64_gcc530/lcg/root/6.06.08-giojec/include/TMVA/Reader.h:1:1: warning: null character(s) ignored

cmsbuild · 2016-10-21T01:06:29Z

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

Dr15Jones · 2016-10-21T02:14:45Z

please test

cmsbuild · 2016-10-21T02:15:00Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15869/console

cmsbuild · 2016-10-21T07:43:18Z

-1

Tested at: d29f764

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16292/15869/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

knoepfel · 2016-10-21T13:49:16Z

@Dr15Jones Changes made and pushed to this fork.

Dr15Jones · 2016-10-21T13:53:07Z

please test

Dr15Jones · 2016-10-21T13:53:48Z

@smuzaffar The reason the previous test report said it failed, is at the end of the log it had

>> Tests for package DQMServices/Components ran.
config/SCRAM/GMake/Makefile.rules:1965: recipe for target 'unittests_TestPoolInput' failed
gmake: *** [unittests_TestPoolInput] Terminated

any idea what that means?

cmsbuild · 2016-10-21T13:57:37Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15883/console
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15890/console

cmsbuild · 2016-10-21T13:57:38Z

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

smuzaffar · 2016-10-21T14:02:22Z

For PR tests we have 2 hours for unittests to finish. As this PR is rebuilding a lot of packages so it could easily have hit the timeout. I am changing the limits now based on the checked out packages.

cmsbuild · 2016-10-21T21:48:42Z

+1
Tested at: abe3676
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16292/15890/summary.html

cmsbuild · 2016-10-21T21:48:45Z

Comparison job queued.

cmsbuild · 2016-10-21T23:03:47Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16292/15890/summary.html

Dr15Jones · 2016-10-22T02:33:02Z

+1

cmsbuild · 2016-10-22T02:33:19Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

davidlange6 · 2016-10-25T09:49:02Z

+1

knoepfel added 2 commits October 20, 2016 15:59

Fix only spacing errors.

d29f764

cmsbuild added this to the Next CMSSW_8_1_X milestone Oct 20, 2016

cmsbuild added comparison-pending core-pending orp-pending pending-signatures tests-pending labels Oct 20, 2016

Dr15Jones suggested changes Oct 20, 2016

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Oct 20, 2016

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Oct 21, 2016

cmsbuild added comparison-pending tests-pending and removed comparison-notrun tests-rejected labels Oct 21, 2016

cmsbuild added tests-started and removed tests-pending labels Oct 21, 2016

Adjustments based on Chris's suggestions.

abe3676

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-rejected labels Oct 21, 2016

cmsbuild added tests-started and removed tests-pending labels Oct 21, 2016

cmsbuild added tests-approved and removed tests-started labels Oct 21, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 21, 2016

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Oct 22, 2016

cmsbuild added orp-approved and removed orp-pending labels Oct 25, 2016

cmsbuild merged commit 1ba792a into cms-sw:CMSSW_8_1_X Oct 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaning up subProcesses looping. #16292

Cleaning up subProcesses looping. #16292

knoepfel commented Oct 20, 2016

cmsbuild commented Oct 20, 2016

Dr15Jones Oct 20, 2016

knoepfel Oct 21, 2016

Dr15Jones Oct 20, 2016

Dr15Jones Oct 20, 2016

knoepfel Oct 21, 2016 •

edited

Dr15Jones Oct 21, 2016

knoepfel Oct 21, 2016

knoepfel Oct 21, 2016

Dr15Jones Oct 20, 2016

knoepfel Oct 21, 2016

Dr15Jones Oct 20, 2016

knoepfel Oct 21, 2016

Dr15Jones Oct 21, 2016

Dr15Jones commented Oct 20, 2016

cmsbuild commented Oct 20, 2016 •

edited

Dr15Jones commented Oct 20, 2016

cmsbuild commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

Dr15Jones commented Oct 21, 2016

cmsbuild commented Oct 21, 2016 •

edited

cmsbuild commented Oct 21, 2016

knoepfel commented Oct 21, 2016

Dr15Jones commented Oct 21, 2016

Dr15Jones commented Oct 21, 2016

cmsbuild commented Oct 21, 2016 •

edited

cmsbuild commented Oct 21, 2016

smuzaffar commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

Dr15Jones commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

davidlange6 commented Oct 25, 2016

Cleaning up subProcesses looping. #16292

Cleaning up subProcesses looping. #16292

Conversation

knoepfel commented Oct 20, 2016

cmsbuild commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knoepfel Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr15Jones commented Oct 20, 2016

cmsbuild commented Oct 20, 2016 • edited

Dr15Jones commented Oct 20, 2016

cmsbuild commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

Dr15Jones commented Oct 21, 2016

cmsbuild commented Oct 21, 2016 • edited

cmsbuild commented Oct 21, 2016

knoepfel commented Oct 21, 2016

Dr15Jones commented Oct 21, 2016

Dr15Jones commented Oct 21, 2016

cmsbuild commented Oct 21, 2016 • edited

cmsbuild commented Oct 21, 2016

smuzaffar commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

cmsbuild commented Oct 21, 2016

Dr15Jones commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

davidlange6 commented Oct 25, 2016

knoepfel Oct 21, 2016 •

edited

cmsbuild commented Oct 20, 2016 •

edited

cmsbuild commented Oct 21, 2016 •

edited

cmsbuild commented Oct 21, 2016 •

edited