Skip to content
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

Backport of tbb cleanup #291

Merged
merged 6 commits into from
Jan 14, 2021

Conversation

dan131riley
Copy link
Collaborator

This PR backports the TBB cleanup to the v2.0.1_patches branch in order to address issue #290 updating TBB. The simplest way to do the backport was to merge most of the CUDA removal PR #283 and all of the FullVector removal PR #286 so besides the TBB update it also removes a lot of dead code.

There shouldn't be an change in physics or computing performance, but I'll try running validation at Cornell (the input file for this older version is no longer in place on phi3).

@dan131riley
Copy link
Collaborator Author

Benchmarks are here. These were run on lnx7188, so the CPU benchmarks may differ some, and the multiple events in flight show a known intermittent anomaly on the first point. Physics should be identical (and do appear to be) to the #240 validation plots.

@slava77
Copy link
Collaborator

slava77 commented Jan 14, 2021

considering the amount of changes, I think that this will become v2.1.0 release once merged, instead of v2.1.0-2 patch.

@slava77
Copy link
Collaborator

slava77 commented Jan 14, 2021

RecoTracker/MkFit/plugins/MkFitProducer.cc:86:41: error: no matching function for call to 'mkfit::MkBuilderWrapper::populate(bool&)'
   86 |   mkfit::MkBuilderWrapper::populate(isFV);

I guess it either requires a PR for CMSSW or this backport should be made more targeted to be compatible with the CMSSW release.

@makortel
Copy link
Collaborator

revert interface change

I suspect that is not sufficient because of

  } else if (build == "fullVector") {
    isFV = true;
    buildFunction_ = mkfit::runBuildingTestPlexFV;

in https://github.com/cms-sw/cmssw/blob/eb134e644ee5255fc0656f1e4181bfca7203306e/RecoTracker/MkFit/plugins/MkFitProducer.cc#L63-L65
We still seem to declare the function

double runBuildingTestPlexFV(Event& ev, MkBuilder& builder);

but could missing the definition lead to link failure?

@dan131riley
Copy link
Collaborator Author

Yes, it does lead to a link failure. (I've now done the test build with CMSSW that I should have before making the PR.)

@dan131riley
Copy link
Collaborator Author

I've verified that the CMSSW MkFit plugin compiles and links with the latest two commits, apologies for not testing that before making the PR. I could make the backport more targeted, but it will take more time. (That we're still declaring runBuildingTestPlexFV in devel is a bug...)

@makortel
Copy link
Collaborator

Just to say it out loud, I'm thinking to remove the use of FullVector from CMSSW side quickly after the mkFit external gets updated (and update https://github.com/trackreco/cmssw as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants