Adding a configurable "unique_id" to DofObject. Currently works for Ser... #133

Merged
merged 1 commit into from Oct 1, 2013

3 participants

@permcody
libMesh - C++ Finite Element Library member

2dproblem
...ialMesh.

figure_1

There is still more work to do before this will work with ParallelMesh, but I just wanted to post this now to get any "architecture" comments out of the way since this touches some pretty important pieces of the library.

@benkirk
libMesh - C++ Finite Element Library member
@roystgnr roystgnr commented on the diff Aug 27, 2013
include/parallel/parallel_implementation.h
@@ -104,6 +104,7 @@ struct Attributes<cxxtype<T> > \
INT_TYPE(unsigned int,MPI_UNSIGNED);
INT_TYPE(long,MPI_LONG);
INT_TYPE(unsigned long,MPI_UNSIGNED_LONG);
+INT_TYPE(unsigned long long,MPI_LONG_LONG_INT);
@roystgnr
libMesh - C++ Finite Element Library member
roystgnr added a line comment Aug 27, 2013

What MPI stacks has this been tested with? I know it's in the standard, but I vaguely recall some incompatibility with long long in a popular MPI implementation when once upon a time I checked.

@permcody
libMesh - C++ Finite Element Library member
permcody added a line comment Aug 28, 2013

Only MPICH2 so far. It works fine there, but I'm just getting started. I'll try MVAPICH and OpenMPI before I post the final patches.

@benkirk
libMesh - C++ Finite Element Library member
benkirk added a line comment Oct 1, 2013

I have yet to test with SGI's MPT, but would be comfortable doing so after a merge.
This looks promising anyway.
http://www.cs.uiuc.edu/homes/wgropp/projects/parallel/MPI/mpi-errata/discuss/longlong/longlong-1-clean.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@roystgnr roystgnr and 1 other commented on an outdated diff Aug 27, 2013
src/parallel/parallel_elem.C
@@ -168,6 +168,12 @@ void pack (const Elem* elem,
data.push_back (elem->subdomain_id());
data.push_back (elem->id());
+#ifdef LIBMESH_ENABLE_UNIQUE_DOF_ID
+ data.push_back (static_cast<largest_id_type>(elem->unique_id()));
+#else
+ data.push_back (0);
@roystgnr
libMesh - C++ Finite Element Library member
roystgnr added a line comment Aug 27, 2013

I think I'd prefer not to send a bunch of 0s around in cases where we're not enabling unique dof ids. However, since changing the buffer sizes at configure time reduces the test coverage of any one configuration I'll understand if others disagree.

@permcody
libMesh - C++ Finite Element Library member
permcody added a line comment Aug 28, 2013

I agree with you, there are additional changes like messing with the header size as well at configure time, but the savings might be worth it. Again, we'll need more testing to understand the cost of this addition, but I'll take that into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@roystgnr roystgnr and 1 other commented on an outdated diff Aug 27, 2013
src/partitioning/partitioner.C
@@ -105,6 +105,11 @@ void Partitioner::partition (MeshBase& mesh,
MeshTools::libmesh_assert_valid_procids<Elem>(mesh);
#endif
+#ifdef LIBMESH_ENABLE_UNIQUE_DOF_ID
+ // Assign DOF object unique ids
+ mesh.assign_unique_ids();
@roystgnr
libMesh - C++ Finite Element Library member
roystgnr added a line comment Aug 27, 2013

So if we've turned on MeshBase::skip_repartitioning(), then newly generated DoFObjects don't get unique ids? That doesn't sound right.

Should the assign_unique_ids() call go into prepare_for_use() instead?

@permcody
libMesh - C++ Finite Element Library member
permcody added a line comment Aug 28, 2013

Good question, Derek and John weren't completely sure of the right place for this call when we went through all the various places things change: i.e. Parallel vs Serial mesh * serial vs parallel runs * mesh adaptivity... I'm not aware of any one place that catches all newly created DOFs. If there is one, please enlighten me. As it stands, it is "safe" but wasteful to call this function multiple times since it won't generate new unique IDs if they already exist for the active DOF objects. This could get even more expensive with ParallelMesh though. I plan to look at this more when I get back to this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@roystgnr roystgnr and 1 other commented on an outdated diff Aug 27, 2013
src/mesh/xdr_io.C
@@ -983,7 +983,7 @@ void XdrIO::read_serialized_connectivity (Xdr &io, const dof_id_type n_elem)
++it;
Elem *parent =
- (parent_id == static_cast<xdr_id_type>(-1)) ?
+ (parent_id == static_cast<xdr_id_type>(-1)) ?
NULL : mesh.elem(parent_id);
@roystgnr
libMesh - C++ Finite Element Library member
roystgnr added a line comment Aug 27, 2013

The xdr_io.C fixes probably ought to just go straight into master; this isn't unique_id stuff but just a bugfix that you found (thanks!) along the way.

I really need to start stochastic configuration testing with BuildBot. I'd have never caught this myself without trying sizeof(some_other_id_type) > sizeof(dof_id_type), and I'd never have thought to try that.

@permcody
libMesh - C++ Finite Element Library member
permcody added a line comment Aug 28, 2013

Right, That bug popped up immediately when I went to a "largest" size of 8 bytes. I'm not sure how you haven't seen it if you have a configuration testing that size. I'll pull that out and push it to master.

@roystgnr
libMesh - C++ Finite Element Library member
roystgnr added a line comment Aug 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@roystgnr
libMesh - C++ Finite Element Library member

You'll probably need changes in parallel_node.C, not just parallel_elem.C. Not sure why your tests with SerialMesh would have succeeded without the former but not without the latter, though - usually when we ship elements around we ship nodes around with them.

@roystgnr
libMesh - C++ Finite Element Library member

Anyway, that's all my issues. Thanks for doing this. If this works out well with ParallelMesh too (and if we add support for it to restart files), I'd love to rearchitect the orientation-dependent FE types to use unique ids (when available) rather than node positioning. It would be nice to be able to use any element with moving meshes, but right now it's only safe to stick with Lagrange if you're going to be repositioning nodes from one time step to another.

@permcody
libMesh - C++ Finite Element Library member
@permcody
libMesh - C++ Finite Element Library member

Thanks for all the feedback! I know this is a bit rough but just wanted to make sure I was generally on the right track. I foresee quite a few more changes as I work through the ParallelMesh implementation. Alas, Derek has changed my priorities again so this patch will get shelved for the next several weeks. I'll get back to it though!

@permcody
libMesh - C++ Finite Element Library member

I'm planning on getting back to the Unique ID stuff later this week and next... This branch isn't dead yet!

@permcody
libMesh - C++ Finite Element Library member

Gentlemen, I believe this feature implementation is complete. So far I haven't seen any performance impacts of carrying around the extra data on each DOF object. See the memory usage graphs I've attached of a few different problems. I tried 2D and large 3D problems again without any noticeable impact. It works with MPICH2, OpenMPI, and MVAPICH-2 without issue. Unless there are any more issues I need to address, I recommend merging this.

Cody

@roystgnr
libMesh - C++ Finite Element Library member

Is it still working for SerialMesh only?

@permcody
libMesh - C++ Finite Element Library member
@roystgnr
libMesh - C++ Finite Element Library member

Enable it with --enable-everything too?

In any case this looks ready to merge to me. I'll let Ben or John hit the big green button once they're happy.

@benkirk
libMesh - C++ Finite Element Library member
@permcody
libMesh - C++ Finite Element Library member

OK - Works for --enable-everything

@benkirk benkirk merged commit e254684 into libMesh:master Oct 1, 2013
@benkirk
libMesh - C++ Finite Element Library member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment