#1322 Change FieldVector to always default-initialize its contents
Metadata
Property | Value |
---|---|
Reported by | Steffen Müthing (steffen.muething@iwr.uni-heidelberg.de) |
Reported at | Jul 24, 2013 08:52 |
Type | Bug Report |
Version | Git (pre2.4) [autotools] |
Operating System | Unspecified / All |
Last edited by | Christian Engwer (christi@conan.iwr.uni-heidelberg.de) |
Last edited at | Sep 27, 2013 20:57 |
Closed by | Christian Engwer (christi@conan.iwr.uni-heidelberg.de) |
Closed at | Sep 27, 2013 20:57 |
Closed in version | Unknown |
Resolution | Implemented |
Comment |
Description
One of our students here in Heidelberg has written a new assembly mode for the BCRSMatrix that is inspired by the way PETSc / Trilinos do things (provide a guesstimate for the number of nonzeroes and just assemble the matrix) - I'll talk about that in a separate post on the developer list.
During testing (specifically on OS X), we've run into a very unfortunate problem with the FieldVector: As matrix assembly is fundamentally an accumulating operation, the basic pattern is "Get a reference to the relevant matrix entry and add stuff". For this purpose, it is important to start every entry in the matrix with a well-known value (i.e. 0). With the old way of building a BCRSMatrix, that wasn't a problem: You constructed your pattern, initialized the matrix with zero and then did the assembly. Now things get more complicated: As the entries are only created when they are accessed the first time, we need a reliable way to initialize them on the fly.
That's where the current behavior of the FieldVector (specifically its default constructor) is an issue: Because it doesn't explicitly initialize its contents, its behavior depends on the kind of contained type: If it is a non-trivial type, the contained data will be default-initialized (e.g. a FieldVector ), when it is a primitive type (FieldVector), you end up with an uninitialized object. Funnily enough, this isn't an issue with the BCRSMatrix on Linux, because all memory containing FieldVectors in there is dynamically allocated, and apparently glibc always zeroes out memory returned from operator new (even though it doesn't have to). The allocator in OS X isn't as eager - allocations on fresh pages are zeroed out (to avoid cross-program information leaks), but subsequent allocations from partially freed pages are not. That makes for nicely unpredictable behavior and a lot of fun during debugging… ;-)
Long story short: I'd like to have the FieldVector always default-initialize its data array. We did some testing, and the overhead is really absolutely miniscule (and could be further reduced by removing all those explicit "matrix = 0;" statements that tend to follow matrix creation). Christian was concerned about additional overhead in the BCRSMatrix copy constructor, but that should be easy to code around (and, again, is hardly noticeable in the first place).
We can't use an alternative constructor because the data is stored in an array, and arrays always call the default constructor.
For the implementation, I did some performance tests (and looked at generated code, in particular for primitive types). By far the best solution is to use uniform initialization if available: It avoids the two-step process of default-initialization and copy assignment for non-primitive types, and newer compilers generate better code for primitive types than with any of the alternatives. If the compiler doesn't have uniform initialization (GCC has it starting with 4.4), I fall back to a simple std::fill. Other options that I thought about but discarded:
-
array placement new: Generates horrible code for primitive types and creates a potential memory leak for user-defined types that have already allocated memory in their default constructor.
-
std::unitialized_fill: Same problem as above - potential resource leak for user-defined types.
I've attached two patches: The first one adds a new configuration test for initializer_list and uniform initialization, and the second one modifies FieldVector. I hope I didn't screw up the cmake test...