Start to use more forward declarations #1464

d7919 · 2018-12-17T21:33:59Z

Work contributes towards resolving #1134

Alongside potential compilation speed improvements (that I've not observed yet), these changes (particularly once fully rolled out) should hopefully help make things a bit more flexible/easy to extend.

This removes the requirement to include the interpolation header

…declares

d7919 · 2018-12-18T15:49:11Z

In terms of optimising the compilation I've done a bit of timing which shows that including bout_types.hxx has a cost of about 0.8s, which compares to a time to compile a single source file of somewhere between 2 and 6 seconds (for the majority of source files). As bout_types is included (directly or indirectly) in a large fraction of our source files we might be able to get a decent saving by splitting this header into separate parts. About 3/4 of the time comes from defining the maps used to convert enums to strings and the associated utility routines (with the other quarter coming from the includes of language standard libraries).

Just declaring these maps without initialising them in the header cuts the time from 0.8s to about 0.45s. It doesn't sound much but I think it could be a reasonable fraction saving across a full build (note configure+compile on travis takes about 10 minutes, with the tests then taking 2-7 minutes).

… to them

…declares

bendudson · 2018-12-20T17:02:03Z

include/bout/coordinates.hxx

 #include <bout_types.hxx>
+#include <field2d.hxx>
+#include <field3d.hxx>
+#include <fieldperp.hxx>


I'm confused about this... should we use <> for system headers and "" for BOUT++ headers?

I tend to view <> as being something provided by a library (including bout) and "" for local files. Or in other words <> for things in an include path and "" for things that aren't. This is just my interpretation though. I think "" can search more places, but if this is an advantage or a disadvantage may be a matter of opinion.

( I should add "" falls back to <> if the first (implementation defined) search fails -- in practice I think that means most of our uses of "" fall back to <> anyway ).

In practice, there is not much difference. The C standard says both are implementation defined, which the exception that "" explicitly looks for a file and <> for a header, which might not be a file.

For "", gcc searches the current directory first, then paths specified with -iquote, then "a standard list of system paths" which includes those in -I.

For <>, gcc searches the system paths only (including those in -I and -isystem).

I don't think we use -I., so private headers do really need to be "", while the public ones can be <>. Beyond that, I think it is a matter of opinion -- but it would be nice to be consistent! As a note, the gcc docs below say to use "" for "your own program" and <> for "system headers".

https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html

One reason to prefer "" for all BOUT++ headers: clang-format and include-what-you-use can group headers together, and it might be nice to separate the system headers from ours.

…leltransform.hxx

As we provide header guards for boutcomm we can avoid including mpi.h multiple times.

johnomotani · 2018-12-28T14:28:00Z

@d7919 tests are failing at the moment. Looks like maybe you forgot to add the .hxx files for ParallelTransform implementations in 4135a74?

d7919 · 2018-12-30T19:06:21Z

@johnomotani thanks, you're right -- I've now added the new files, thanks!

…declares

ZedThree · 2019-01-03T17:15:40Z

I think it's now just missing #include "dcomplex.hxx" in slepc.cxx

ZedThree · 2019-01-03T17:26:19Z

include/bout/mesh.hxx

-      throw BoutException("Unhandled direction encountered in getAllowedStaggerLoc");
-    }
-  };
+  CELL_LOC getAllowedStaggerLoc(DIRECTION direction) const;


These three functions now can't be inlined -- but it doesn't look like they're used in any tight loops so presumably this isn't that bad

Yes, this is one of the reasons I think this PR might be more for discussion/experimentation to decide on a policy. I'd prefer runtime performance over compilation performance but there may be places we can get both.

ZedThree · 2019-01-03T17:33:05Z

While we're tidying up, we should probably rename all the include guards as macros beginning with __ or _X are technically reserved by the preprocessor.

ZedThree · 2019-01-03T17:37:24Z

include/bout/solver.hxx

@@ -35,17 +35,29 @@

 class Solver;


We should be able to delete this

ZedThree · 2019-01-04T21:38:05Z

I used include-what-you-use plus some manual fiddling to make sure everything in headers is inside include guards: https://travis-ci.org/boutproject/BOUT-dev/builds/475492401

Also made all BOUT++ headers included with "" and used clang-format to group headers together (main header (i.e. field3d.hxx for field3d.cxx-> system headers -> bout/ headers -> everything else)

Any gain over this PR is in the noise, but it would at least be a consistent treatment of headers. At the cost of 2k+ lines changed!

I'm in favour of merging this PR at least, it's about a 20% speedup of compile times on Travis

d7919 · 2019-01-05T21:19:22Z

I used include-what-you-use plus some manual fiddling to make sure everything in headers is inside include guards: https://travis-ci.org/boutproject/BOUT-dev/builds/475492401

Also made all BOUT++ headers included with "" and used clang-format to group headers together (main header (i.e. field3d.hxx for field3d.cxx-> system headers -> bout/ headers -> everything else)

Any gain over this PR is in the noise, but it would at least be a consistent treatment of headers. At the cost of 2k+ lines changed!

I'm in favour of merging this PR at least, it's about a 20% speedup of compile times on Travis

For reference I've made a branch that just includes the changes to bout_types and the travis build is at https://travis-ci.org/boutproject/BOUT-dev/builds/475809020?utm_source=github_status&utm_medium=notification -- this hasn't finished yet but I suspect this should get much of the compilation gain from this PR.

bendudson · 2019-02-21T11:21:01Z

Is this one for tidying and merging, or closing?

ZedThree · 2019-02-21T11:23:26Z

The tidying is likely going to be a nightmare so probably closing and rebasing. I did have a go last week, I'll check it still goes in cleanish.

d7919 added 14 commits December 17, 2018 15:54

Reduce includes in Field.hxx

ebe17f5

Use mostly forward declarations in mesh.hxx

1b6dbef

More forward declarations in Field* headers

45aa4b8

More forward declares in utils

7699551

More forward declares in Datafile

9c170f7

Remove unused include in dataformat

50457d0

More forward declares

28f3d27

More forward declares in field_factory.hxx

f0c4ecf

Forward declares in interpolation and initialprofiles

a6133a2

Some more forward declares

4c579dd

More forward declarations

5cfd5c7

Update tests for change to headers

e5b16fa

Minimise number of times iostream is included

041bf0d

Update boutcore for header changes

13472ee

d7919 added the work in progress Not ready for merging label Dec 17, 2018

d7919 added 5 commits December 17, 2018 23:01

More fiddling with includes

593198c

Update test-interchange for include updates

6a31166

Prefer CELL_LOC_STRING to strLocation in generated_fieldops

7ff486e

This removes the requirement to include the interpolation header

Small tweak to interpolation includes

868d9bc

Merge remote-tracking branch 'refs/remotes/origin/next' into forward_…

15dcf5f

…declares

johnomotani mentioned this pull request Dec 18, 2018

Namespace for globals? #1468

Closed

d7919 added 5 commits December 18, 2018 19:28

Move map and function definitions into source file for bout_types

c5d37d4

Make function internals static in bout_types as we return a reference…

52e72e2

… to them

A few more header tweaks

13964c2

Merge remote-tracking branch 'refs/remotes/origin/next' into forward_…

8415871

…declares

Move getMesh into source file

cd6929c

bendudson reviewed Dec 20, 2018

View reviewed changes

d7919 added 2 commits December 21, 2018 09:32

Move ENUM_STRING into source file

05fdac5

Move paralleltransform signatures into local header rather than paral…

4135a74

…leltransform.hxx

d7919 added 3 commits December 21, 2018 09:57

Move some Mesh member function bodies into source file

469c711

Remove explicit mpi.h includes -- prefer use of boutcomm

3f61984

As we provide header guards for boutcomm we can avoid including mpi.h multiple times.

Small tweaks of bout++.cxx includes

baedb7f

johnomotani mentioned this pull request Dec 28, 2018

Use namespace for global variables 'mesh' and 'dump' #1470

Merged

Add missing header files

f5c6b38

d7919 added 3 commits January 2, 2019 10:45

Fix for moved header component

9b33e46

Fix for moved header component

88936fe

Merge remote-tracking branch 'refs/remotes/origin/next' into forward_…

0968e10

…declares

ZedThree reviewed Jan 3, 2019

View reviewed changes

include/bout/solver.hxx

@@ -35,17 +35,29 @@

class Solver;

Copy link

Member

ZedThree Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to delete this

Add missing include

7f7118d

ZedThree mentioned this pull request Jan 7, 2019

Include what we use (header tidy) #1484

Closed

d7919 mentioned this pull request Jan 22, 2019

Add multiple parallel slices #1345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start to use more forward declarations #1464

Start to use more forward declarations #1464

d7919 commented Dec 17, 2018

d7919 commented Dec 18, 2018

bendudson Dec 20, 2018

d7919 Dec 20, 2018 •

edited

ZedThree Jan 2, 2019

ZedThree Jan 4, 2019

johnomotani commented Dec 28, 2018

d7919 commented Dec 30, 2018

ZedThree commented Jan 3, 2019

ZedThree Jan 3, 2019

d7919 Jan 3, 2019

ZedThree commented Jan 3, 2019

ZedThree Jan 3, 2019

ZedThree commented Jan 4, 2019

d7919 commented Jan 5, 2019

bendudson commented Feb 21, 2019

ZedThree commented Feb 21, 2019

Start to use more forward declarations #1464

Are you sure you want to change the base?

Start to use more forward declarations #1464

Conversation

d7919 commented Dec 17, 2018

d7919 commented Dec 18, 2018

bendudson Dec 20, 2018

Choose a reason for hiding this comment

d7919 Dec 20, 2018 • edited

Choose a reason for hiding this comment

ZedThree Jan 2, 2019

Choose a reason for hiding this comment

ZedThree Jan 4, 2019

Choose a reason for hiding this comment

johnomotani commented Dec 28, 2018

d7919 commented Dec 30, 2018

ZedThree commented Jan 3, 2019

ZedThree Jan 3, 2019

Choose a reason for hiding this comment

d7919 Jan 3, 2019

Choose a reason for hiding this comment

ZedThree commented Jan 3, 2019

ZedThree Jan 3, 2019

Choose a reason for hiding this comment

ZedThree commented Jan 4, 2019

d7919 commented Jan 5, 2019

bendudson commented Feb 21, 2019

ZedThree commented Feb 21, 2019

d7919 Dec 20, 2018 •

edited