Topic/datatype

[bosilca]

This started as a fix for #3439 but then evolved into a complete redesign of the handling of some datatype and convertor internal structures (mainly the array of counts for predefined types).

[bosilca]

@ggouaillardet can you try this in a heterogeneous environment ?

[ibm-ompi]

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/805cc76376493b5d175dca9ac64833a2

[ibm-ompi]

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/d227a5018dfdd6cb9d3336835b9338bd

[ibm-ompi]

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/30256b2e57e9a3a40fc8ef555877a110

[ggouaillardet]

@bosilca this PR build but all tests crash
i will be off until Monday, and i will unlikely have any chance to review this before.
once this is fixed, do you plan to backport it to the release branches ? or make a simpler fix just for the release branches ?

[bosilca]

I haven't checked yet, but I think this patch would allow us to drastically reduce the size of the predefined datatypes. If we implement the reduction, we will change the ABI, so this might only be good for 3.0. For the others I will take a look at having a slimmed down version.

[ibm-ompi]

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/2fce5a06ccb4313d246c882baaeb5239

[ibm-ompi]

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/90bfae8e90e611662bc9637eb7aca130

[ibm-ompi]

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/cc4639d8b917a0e8d3bac6fd2ed0dbd2

[ibm-ompi]

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/6fd8c492a06afe924bcdeedd43ed6d5c

[ibm-ompi]

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/f4a14ddc5fca9a636699d7d67d76ce26

[ibm-ompi]

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/4d387051d59b342b93d537dea980d4c3

[bosilca]

It turns out that in addition of being a bug fix, this patch also provide a performance boost. We have shred few nsecs from our shared memory latency from 95.450 nsecs to 91.865 nsecs on a E5-2650 v3 @ 2.30GHz.

[ggouaillardet]

datatype/getel and datatype/darray-pack from the ibm test suite fail. possible fixes are in the inline comments

[ggouaillardet]

did you really mean to commit changes to this file ?
before entering the loop, the st_offsets array is uninitialized, and this commit causes a failure in the datatype/darray-pack test from the ibm test suite

[bosilca]

this was not supposed to get pushed. It is from another patch, but it is wrong and incomplete. I will remove.

[ggouaillardet]

the datatype/getel test from the ibm test suite loops forever.

i got things working with the patch below.
opal_datatype_get_element_count might have to be updated as well, though i could not write a program that causes the same endless loop

diff --git a/opal/datatype/opal_datatype_get_count.c b/opal/datatype/opal_datatype_get_count.c
index a860d5f..c7c87ea 100644
--- a/opal/datatype/opal_datatype_get_count.c
+++ b/opal/datatype/opal_datatype_get_count.c
@@ -169,10 +169,13 @@ int opal_datatype_compute_ptypes( opal_datatype_t* datatype )
     while( 1 ) {  /* loop forever the exit condition is on the last OPAL_DATATYPE_END_LOOP */
         if( OPAL_DATATYPE_END_LOOP == pElems[pos_desc].elem.common.type ) { /* end of the current loop */
             if( --(pStack->count) == 0 ) { /* end of loop */
-                stack_pos--; pStack--;
-                if( stack_pos == -1 ) return 0;  /* completed */
+                if( stack_pos == 0 ) return 0;  /* completed */
+                stack_pos--;
+                pStack--;
+                pos_desc++;
+            } else {
+                pos_desc = pStack->index + 1;
             }
-            pos_desc = pStack->index + 1;
             continue;
         }
         if( OPAL_DATATYPE_LOOP == pElems[pos_desc].elem.common.type ) {

[bosilca]

Thanks for noticing. The same pattern appears in several location, I fixed them all. Please review the updated version.

[ggouaillardet]

@bosilca this looks good to me
i also tried in an hetero environment and could not find any issue

[jsquyres]

bot:ompi:retest

[jsquyres]

Note that per discussions, this PR caused master to become ABI-incompatible with the v3.x (which will shortly be renamed to be v3.0.x) branch. That's not a problem -- it just means that the next release series with be v4.0.x (vs. v3.1.x.).

[bosilca]

We have not released yet the 3.0. Why not including this change before the official release? This PR is not only a performance fix, but it addresses a real issue (identified by Gilles in #3439).

[jsquyres]

@bwbarrett @hppritcha What say you for v3.0.0?

[bwbarrett]

I'm not happy about a big DDT patch 2 weeks before we do our first RC. Would have been better to fix the bug without all the other work, but I suppose I'm not going to have a choice, am I?

[bosilca]

I like your pragmatism ;) In the defense of the patch, it looks big because it changes all the accesses to a datatype field (from ".btypes" into "->ptypes"), but the logical changes are rather small.

Technically it is possible to make a patch for the 3.x that does not have the dynamic ptypes array. This will address the problem Gilles found, but: 1) the code will diverge between 3.x and master (and additional issues might be difficult to fix); 2) it will also require 47 * sizeof(size_t) extra bytes per datatype (including the predefined); and 3) the homogeneous case will not be as streamlined as with this patch. Your call.

[hppritcha]

I tried to apply this patch to v3.x and it doesn't patch cleanly.

pn1249323:~/ompi (v3.x)$ git am patch_file
Applying: Don't overflow the internal datatype count. Change the type of the count to be a size_t (it does not alter the total size of the internal structures, so has no impact on the ABI).
error: patch failed: opal/datatype/opal_datatype_internal.h:155
error: opal/datatype/opal_datatype_internal.h: patch does not apply
error: patch failed: opal/datatype/opal_datatype_optimize.c:50
error: opal/datatype/opal_datatype_optimize.c: patch does not apply
Patch failed at 0001 Don't overflow the internal datatype count. Change the type of the count to be a size_t (it does not alter the total size of the internal structures, so has no impact on the ABI).
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
pn1249323:~/ompi (v3.x|AM 1/6)$ git am --abort
pn1249323:~/ompi (v3.x)$ git show HEAD

[bosilca]

Indeed, we didn't made the change from OPAL_PTRDIFF_TYPE to ptrdiff_t in master. I have prepared a PR for you. #3504