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

Explore ways to reduce number of recursive includes in Algorithm.h with desire of speeding up builds #15246

Closed
FedeMPouzols opened this issue Feb 10, 2016 · 4 comments
Assignees
Labels
Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period.
Milestone

Comments

@FedeMPouzols
Copy link
Contributor

Try to decrease the number of files included from Algorithm.h (apparently over 150), most of which come from MatrixWorkspace.h (some 140).

This stems from the ranking and plots of number of includes and compilation time made by @SimonHeybrock .

@martyngigg

@FedeMPouzols FedeMPouzols added Maintenance Unassigned issues to be addressed in the next maintenance period. Framework Issues and pull requests related to components in the Framework labels Feb 10, 2016
@FedeMPouzols FedeMPouzols self-assigned this Feb 10, 2016
@FedeMPouzols FedeMPouzols added this to the Release 3.7 milestone Feb 10, 2016
FedeMPouzols added a commit that referenced this issue Feb 11, 2016
don't include: Axis, RunSample, SpectraDetectorTypes, EmptyValues
@SimonHeybrock
Copy link
Contributor

@FedeMPouzols I have been looking at quite a few algorithms and would like to move things like getting L1 and L2 into a separate class. As a consequence these algorithms would no longer need to know about instrument and detectors, which might speed up compile times.

Thus, I would like to propose the following:

It is hard to just replace the Instrument.h (or Detector.h) includes by forward declarations, since that will break hundreds of algorithms, and would take forever to fix. So, to do that step by step:

  1. Move Algorithm.h to AlgorithmMinimalIncludes.h. Move all unnecessary includes from that header into Algorithm.h (which now just includes AlgorithmMinimalIncludes.h and some others but no actual code). Put forward declarations into AlgorithmMinimalIncludes.h.
  2. Algorithm.cpp includes AlgorithmMinimalIncludes.h and necessary includes.
  3. Remove Instrument.h and related includes from upstream of AlgorithmMinimalIncludes.h (put forward declarations). The goals is to be able to include AlgorithmMinimalIncludes.h without pulling in anything from Instrument.
  4. Existing algorithms still get all includes via Algorithm.h.
  5. Algorithms we modify can include AlgorithmMinimalIncludes.h and just include themselves what they need.

Point 5.) is what I plan to do with some algorithm refactoring I have in mind.

@FedeMPouzols
Copy link
Contributor Author

@SimonHeybrock : That sounds like a plan. What headers included from Algorithm.h do you think can/should go away?

Some related points:

  • There's also the consideration that there are quite a few places in the code, including interfaces and tests, where they wouldn't need to include Algorithm.h as they could just use IAlgorithm. I was thinking that one of the reasons behind the includes-mess that we have is that interfaces haven't been used to their full potential.
  • Many headers are currently including a lot of stuff from Geometry via MatrixWorkspace. I've found that unnecessary in most places (where IDTypes.h or in a few cases one or two small includes from Geometry are enough).
  • In this issue in the end I'm concentrating on MatrixWorkspace.h for now, which would become much more lightweight. I took MatrixWorkspace.h out of WorkspaceFactory.h (replaced by its forward declaration). But I think I haven't even touched Algorithm.h directly. There are very few places that really need Sample, Run, ISpectrum, Unit, etc. It is definitely very painful, but I think it is worth a try. I'm almost there with this MatrixWorkspace.h stripping effort. Let's see if it brings any noticeable improvement in compilation times. It will be definitely good as a way to disentangle our overcomplicated network of includes a bit, and it should help in further code reorganization.
  • I still think that separate headers for the forward declarations that we already have for things like Instrument and IComponent, as we already added some months ago for the workspace types, would be good for the sake of organization. But my first guess is that that would be a lower priority. The changes required are easier to automate. I've opened an issue for that: Add more forward declarations for typedefs in Kernel and Geometry #15268.

@SimonHeybrock
Copy link
Contributor

@FedeMPouzols Sounds good!

What should be in Algorithm.h? Good question.

  • Instruments, Detectors, or anything like that (basically nothing from Geometry?) should go away.
  • Property-related stuff should probably be in there?

@FedeMPouzols
Copy link
Contributor Author

In the first commits I've been attacking the includes behind MatrixWorkspace.h (MatrixWorkspace.h was included in WorkspaceFactory.h, included from Algorithm.h). It is now replaced by MatrixWorkspace_fwd.h. This required many updates everywhere in the code but it seems to have a very positive effect on the excessive number of recursive includes and compilation time. I think it also makes it more explicit what algorithms, interfaces, etc. use or manipulate MatrixWorkspace Axis, etc. objects.

MatrixWorkspace.h is much more lightweight now. It doesn't include these headers anymore: Axis.h, Run.h, Sample.h, SpectraDetectorTypes.h EmptyValues.h.

Also, wherever possible I replaced includes with forward declarations like for example:

namespace Geometry { class Object; }
namespace API { class Sample; }

Then I introduced _fwd.h forward declare headers for Instrument and IDetector. And the last couple of commits remove WorkspaceFactory.h from Algorithm.h.

All this has reduced the average number of (recursive) includes in Framework cpp files from 114 to 103.

Before:

Algorithm.h: 155 includes
MatrixWorkspace.h: 138 includes
ExperimentInfo.h: 91 includes

After:

Algorithm.h: 79 includes
MatrixWorkspace.h: 119 includes
ExperimentInfo.h: 69

Before:

Number of cpp files                          :   1531
Total recursive number of includes in files  : 175729
Average number of includes in cpp files      :    114
Total compile time                           : 6155.4

After:

Number of cpp files                          :   1531
Total recursive number of includes in files  : 159200
Average number of includes in cpp files      :    103
Total compile time                           : 5937.4

Note that the compile time includes all the targets and tests. Real build times with a cmake/ninja build for Framework on my debian box (4 cores):

Before:

5932.38user 241.55system 26:42.91elapsed 385%CPU (0avgtext+0avgdata 1463428maxresident)k
136inputs+3354808outputs (0major+178496730minor)pagefaults 0swaps

After:

5665.98user 236.36system 25:35.59elapsed 384%CPU (0avgtext+0avgdata 1399252maxresident)k
408inputs+3355232outputs (0major+173762617minor)pagefaults 0swaps

I think there's still a lot to improve behind MatrixWorkspace, ExperimentInfo and related headers (I'm creating a follow-up issue). I'd stop this branch here as there are already lots of changes and it is highly prone to develop conflicts (as it is already happening).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
None yet
Development

No branches or pull requests

2 participants