#1729: Switch Carpet to new bboxset class implementation -------------------------+-------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: new Priority: major | Milestone: Component: Carpet | Version: development version Keywords: | -------------------------+-------------------------------------------------- Carpet has a new bboxset class implementation that scales to much larger number of MPI processes. I suggest to make the new implementation the default.
This is currently disabled, and can be enabled by adding the two flags "-DCARPET_ENABLE_BBOXSET2 -DCARPET_USE_BBOXSET2" to CPPFLAGS.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: new => review
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
I seem to remember that this new class required C++11. Is that the case? bboxset2.hh seems to have some workarounds in case lambdas are not available, so maybe this is no longer the case?
I have glanced at the code, but do not have time to do a detailed code review, as there is a lot of code there. I assume that if it passes the current tests, it is appropriate to make this the default at this point in the release cycle, and if there are problems, that change can be reverted before the next release. If you feel this level of review is appropriate, please go ahead.
One comment: this is the sort of code for which it is very easy to write unit tests. Assuming they don't already exist, where should they go? I have some local additions to the old bboxset code, and wanted to write tests for them, but wasn't sure of the best way of organising that.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
Yes, this requires C++11. There are very few systems where C++11 is not available, and there one needs to use a -DCARPET_DISABLE_BBOXSET2 to fall back to the previous implementation (hello, Zwicky!).
Yes, there are unit tests. They are in the thorn Carpet/TestBboxSet2.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
What sort of error message would people get at the moment if C++11 is not available, and they try to build the ET on a machine without the CARPET_DISABLE_BBOXSET2 macro defined? Should C++11 be detected automatically duing autoconf, and Carpet use this to decide whether the new code is used? Or should the check be more specific, for the features which are actually used?
I am thinking of users trying to set up the ET on a new machine which does not have C++11 (are these sufficiently rare that we can assume users won't encounter them?) and getting an error message which does not clearly explain that they need to set a Carpet macro due to having an old compiler.
Thanks for the unit tests; I actually looked for such a thorn, but for some reason didn't see it.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
The error message would likely (so I hope) complain about a C++11 feature not available, such as a range-based for loop or a lambda. If the compiler is really old, it may just be a syntax error. An autoconf macro would be nice.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
According to http://www.stroustrup.com/C++11FAQ.html#11, we could test the {{{__cplusplus}}} macro. See also http://stackoverflow.com/questions/5047971/how-do-i-check-for-c11-support.
{{{ #if __cplusplus <= 199711L #error This library needs at least a C++11 compliant compiler #endif }}}
Maybe we could try this?
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by rhaas):
Replying to [comment:3 eschnett]:
Yes, this requires C++11. There are very few systems where C++11 is not
available, and there one needs to use a -DCARPET_DISABLE_BBOXSET2 to fall back to the previous implementation (hello, Zwicky!). Please have a look at simfactory2's current master and the zwicky-intel14 set of files in there.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by knarf):
Replying to [comment:6 hinder]:
According to http://www.stroustrup.com/C++11FAQ.html#11, we could test
the {{{__cplusplus}}} macro. See also http://stackoverflow.com/questions/5047971/how-do-i-check-for-c11-support.
{{{ #if __cplusplus <= 199711L #error This library needs at least a C++11 compliant compiler #endif }}}
Maybe we could try this?
If such a way to test for C++11 is available, then we should not output an error in that case, but define CARPET_DISABLE_BBOXSET2 instead (in addition to a warning maybe), shouldn't we?
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
I wasn't suggesting that code fragment literally; I was using it as a way to illustrate how C++11 can be checked for. On the other hand, is it confusing to have the version of the code used depend on the version of the compiler? There should probably at least be a warning at compile time such as "C++11 not found; falling back to old bboxset implementation", so that it is clear that the preferred version is the C++11 version, and using the old version is not recommended in some sense.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
[Sorry Frank, I didn't read your comment carefully enough, and missed that you had already mentioned a warning.]
To push this ticket forward, let me make a concrete suggestion:
If the user sets the CARPET_DISABLE_BBOXSET2 macro or {{{if __cplusplus <= 199711L}}}, then fall back to the old bboxset implementation and emit a compile-time warning that the old implementation is being used because C++11 is not available.
This should mean that people don't have to explicitly configure for this feature on their machine. Then, if the tests are OK, as they apparently are, then I am happy for this to be made the default at this point in the release cycle.
#1729: Switch Carpet to new bboxset class implementation --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: closed Priority: major | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: review => closed * resolution: => fixed
Comment:
I implemented this not by checking the reported standard version, but by adding autoconf macros for the required features. I also added a build- time warning if Carpet disables the new bboxset class.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit