Skip to content

Conversation

@permcody
Copy link
Member

The patch fixes up issues with writing 64-bit types to XDA/XDR files. Note that I am not enabling 64-bit mode with this patch, just fixing up issues.

Copy link
Member

Choose a reason for hiding this comment

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

I was about to say "take this back out", but actually it makes a pretty good shorthand for "we don't have a configure option for this yet".

Copy link
Member Author

Choose a reason for hiding this comment

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

I can if you'd like, but that brings up the next question...

My next patch that adds unique_ids and named entities is already making
several changes to the existing format. It'll have a new version number,
"libmesh-0.9.2+" but will still handle version 0.7.0 formats just fine.
Since UniqueIDs are going to default to 8-bytes automatically is it time to
hit the switch and default XDR format to 64-bit? There really isn't much
of a change when using XDA, but XDR files will be almost twice as big. It
is 2013 and these files are still plenty small, thoughts?

Alternatively, I could allow the user to configure that option and override
it if any of the stored types are sized to 64-bit. The only issue is
that we'll have the potential for people in the same organization to
produce different sized XDR/XDA files... In that case do we need another
flag in the XDR/XDA format (since we are changing everything else anyway)
to store the "stored" encoding size?

Thoughts?
Cody

On Fri, Oct 11, 2013 at 12:56 PM, roystgnr notifications@github.com wrote:

In src/mesh/xdr_io.C:

@@ -41,6 +41,7 @@
namespace libMesh
{

+//#define LIBMESH_USE_64BIT_XDR

I was about to say "take this back out", but actually it makes a pretty
good shorthand for "we don't have a configure option for this yet".


Reply to this email directly or view it on GitHubhttps://github.com//pull/156/files#r6923719
.

Copy link
Member

Choose a reason for hiding this comment

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

My personal preference would be to add not just one "stored encoding size" to the header, but one per data type. Then we could actually store unpadded 4 byte dof ids, 2 byte subdomain ids, etc. even in cases which need an 8 byte unique id. That's a significant bit of work, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that would give us the most efficient packing but would require a fair
amount of rework indeed with all of the chunked I/O. Sounds like a good CS
student summer project!

Since I'm not too inclined to bite that off right now, how about I define
"xdr_id_type" to match "largest_id_type" and put in that single encoding
value in the header? That'll allow those of us who are going to start
using unique_ids the ability to move to a 64-bit file encoding while
leaving everything else status quo for those using default libMesh
configuration options. Once that's in place, it's just be plug'n'chug for
somebody who has a bit of free time to pack things tightly! For the most
part, we are still using XDA format so we aren't too concerned about file
size just yet.

Cody

On Fri, Oct 11, 2013 at 1:17 PM, roystgnr notifications@github.com wrote:

In src/mesh/xdr_io.C:

@@ -41,6 +41,7 @@
namespace libMesh
{

+//#define LIBMESH_USE_64BIT_XDR

My personal preference would be to add not just one "stored encoding size"
to the header, but one per data type. Then we could actually store unpadded
4 byte dof ids, 2 byte subdomain ids, etc. even in cases which need an 8
byte unique id. That's a significant bit of work, though.


Reply to this email directly or view it on GitHubhttps://github.com//pull/156/files#r6924203
.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... xdr_id_type matching largest_id_type is definitely the way to start. But what I'd do is put those stored encoding sizes in the new format version's header anyway, write them out with sizeof(xdr_id_type), when when reading them back in assert that they match sizeof(xdr_id_type). That will at least make it easier for people to figure the problem out once the inevitable "he wrote this file with 4 byte ints, but I'm configured to use 8 byte ints" or vice-versa occurs, and it'll mean that we can make the reader and writer code more flexible later without requiring another format change.

Copy link
Member Author

Choose a reason for hiding this comment

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

On Fri, Oct 11, 2013 at 1:47 PM, roystgnr notifications@github.com wrote:

In src/mesh/xdr_io.C:

@@ -41,6 +41,7 @@
namespace libMesh
{

+//#define LIBMESH_USE_64BIT_XDR

Hmm... xdr_id_type matching largest_id_type is definitely the way to
start. But what I'd do is put those stored encoding sizes

One for each type? So my less efficient version will look like this:

assuming they are using libMesh defaults so largest_id_type = 4

4 # boundary_id_type
4 # dof_id_type
4 # unique_id_type (if this is turned off, it'll still get written
but won't participate in the size selection)
4 # processor_id_type
4 # subdomain_id_type

This is essentially what's happening right now anyway so now I'm just
making it explicit. I'll get this all coded up and put out a pull request.
If you want, I could optionally output unique_ids, we aren't really doing
this with any of the rest of the connectivity fields in practice, but it
wouldn't be terribly difficult to handle and detect, just one more field in
the header saying which fields are written.

in the new format version's header anyway, write them out with

sizeof(xdr_id_type), when when reading them back in assert that they match
sizeof(xdr_id_type). That will at least make it easier for people to figure
the problem out once the inevitable "he wrote this file with 4 byte ints,
but I'm configured to use 8 byte ints" or vice-versa occurs, and it'll mean
that we can make the reader and writer code more flexible later without
requiring another format change.


Reply to this email directly or view it on GitHubhttps://github.com//pull/156/files#r6924926
.

Copy link
Member

Choose a reason for hiding this comment

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

Hm... we ought to eventually make sure that configurations with unique ids can read in files written without them, and even that configurations without them can read in files written with them (except without the actual ids, obviously)... and the sizeof(unique_id_type) header could handle part of that. Just make that "0" if unique ids aren't in the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - I'll do all that with this patch... Lots of cases to test!

On Fri, Oct 11, 2013 at 2:05 PM, roystgnr notifications@github.com wrote:

In src/mesh/xdr_io.C:

@@ -41,6 +41,7 @@
namespace libMesh
{

+//#define LIBMESH_USE_64BIT_XDR

Hm... we ought to eventually make sure that configurations with unique ids
can read in files written without them, and even that configurations
without them can read in files written with them (except without the actual
ids, obviously)... and the sizeof(unique_id_type) header could handle part
of that. Just make that "0" if unique ids aren't in the file.


Reply to this email directly or view it on GitHubhttps://github.com//pull/156/files#r6925380
.

@roystgnr
Copy link
Member

Unless anyone else has objections, I'd say merge this as is. All the "future xdr version" speculative stuff we discussed above can go in a subsequent PR.

permcody added a commit that referenced this pull request Oct 11, 2013
Make LIBMESH_USE_64BIT_XDR work!
@permcody permcody merged commit 692fd16 into libMesh:master Oct 11, 2013
@permcody permcody deleted the XDR_64 branch October 11, 2013 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants