-
Notifications
You must be signed in to change notification settings - Fork 300
Make LIBMESH_USE_64BIT_XDR work! #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: