Skip to content

Conversation

bosilca
Copy link
Member

@bosilca bosilca commented Nov 9, 2016

Skip the refcount update via an atomic operation for all predefined datatypes. This saves 2 atomics on the critical path for all communications, as long as the datatype is predefined.

@bosilca bosilca added this to the v2.0.3 milestone Nov 9, 2016
@bosilca bosilca force-pushed the topic/no_predefined_ddt_refcount branch from 6fdb361 to 805eaec Compare November 9, 2016 23:15
#define OMPI_DATATYPE_RELEASE(ddt) \
{ \
if( !ompi_datatype_is_predefined((ddt)) ) { \
OBJ_RELEASE((ddt)); \
Copy link
Contributor

Choose a reason for hiding this comment

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

@bosilca you need to OPAL_OUTPUT_VERBOSE __before``` OBJ_RELEASE
also, is this the right verbosity level here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this up. I increased the debug level to 100.

@bosilca bosilca force-pushed the topic/no_predefined_ddt_refcount branch from 805eaec to 6981757 Compare November 10, 2016 21:06
#define OMPI_DATATYPE_RELEASE(ddt) \
{ \
if( !ompi_datatype_is_predefined((ddt)) ) { \
OPAL_OUTPUT_VERBOSE((0, 100, "Datatype %p [%s] refcount %d in file %s:%d\n", \
Copy link
Contributor

Choose a reason for hiding this comment

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

@bosilca fwiw, you might or might not want to print
(ddt)->super.super.obj_reference_count - 1 instead

i do not really have a strong opinion on that 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.

This message reports non-thread safe data, with the exception of developers nobody should ever use it. Let's keep it as is.

@jjhursey
Copy link
Member

bot:retest

@bosilca bosilca force-pushed the topic/no_predefined_ddt_refcount branch from 6981757 to c2cd717 Compare January 11, 2017 21:49
@jsquyres jsquyres dismissed ggouaillardet’s stale review February 6, 2017 20:20

Gilles indicated that he didn't have a strong opinion about the change he requested.

@jsquyres
Copy link
Member

jsquyres commented Feb 6, 2017

@ggouaillardet Can you re-review this PR?

@jsquyres
Copy link
Member

@ggouaillardet ping

@ggouaillardet ggouaillardet merged commit 4184c01 into open-mpi:master Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants