Skip to content

Conversation

@edgargabriel
Copy link
Member

update the reference counter for predefined datatypes correctly in our internal datatype_duplicate function.

@bosilca
Copy link
Member

bosilca commented Aug 31, 2016

@edgargabriel can we do it in the opposite way, aka. don't release predefined datatypes ?

@edgargabriel
Copy link
Member Author

I thought about that, but that fix would be more intrusive (has to fix file_set_view as well as file_close). In addition, I thought logically it makes sense to increase the refcount on a datatype if we have a reference to it, even if it is a basic datatype.

I can change that however if you feel that it would be better.

@jsquyres
Copy link
Member

I just merged this on the v2.x branch so that we could move forward on the v2.0.1 release. I'm merging it here so that we stay in sync with master. If we want to do something better than this PR, please open a new PR. Thanks!

@jsquyres jsquyres merged commit 16fe18e into open-mpi:master Aug 31, 2016
@ggouaillardet
Copy link
Contributor

+1 on @bosilca suggestion

we already do that (e.g. treat predefined and non predefined datatypes differently) at several places,
plus this is slightly more efficient.

since this is an enhancement/optimization, there is no rush and it can wait v2.0.2

@edgargabriel
Copy link
Member Author

ok, thanks for the feedback @bosilca and @ggouaillardet. I will with draw the pr for master, and redo it along those lines. (That was actually my first solution before I realized that the other solution is a one-liner). We can than decide whether to move it to the 2.0.x series as well, or stick with the current solution for this version.

@ggouaillardet
Copy link
Contributor

@edgargabriel note the PR has already been merged into master and v2.x
you can either revert the commit and then issue a new one, or simply make a single commit that reverts the changes and make new ones

@edgargabriel edgargabriel deleted the pr/datatype-refcount-fix branch July 17, 2017 14:10
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.

4 participants