Opened 10 years ago

Last modified 5 years ago

#288 new bug

Request initialization

Reported by: William Gropp <wgropp@…> Owned by:
Priority: minor Milestone: future
Component: mpich Keywords:

Description (last modified by balaji)

The request creation is on the critical path for latency - the intent
was that the parts of the code that needed these fields was
responsible for setting them before using them.  While these changes
are reasonable temporary fixes, we need to reduce the overhead in
request creation and management.

One starter would be to correct the code that should have set the
fields that are now nulled out here - where are they?


On Nov 13, 2008, at 8:26 AM, wrote:

> Author: goodell
> Date: 2008-11-13 08:26:51 -0600 (Thu, 13 Nov 2008)
> New Revision: 3512
> Modified:
>   mpich2/branches/dev/knem/src/mpid/ch3/include/mpidimpl.h
> Log:
> Merge [bd5d5e2750af7693de0a533a3c5d52df8a441129] from trunk -> knem.  This zeroes out some additional
> fields in MPID_Requests.
> Modified: mpich2/branches/dev/knem/src/mpid/ch3/include/mpidimpl.h
> ===================================================================
> --- mpich2/branches/dev/knem/src/mpid/ch3/include/mpidimpl.h
> 2008-11-13 14:20:26 UTC (rev 3511)
> +++ mpich2/branches/dev/knem/src/mpid/ch3/include/mpidimpl.h
> 2008-11-13 14:26:51 UTC (rev 3512)
> @@ -316,6 +316,7 @@
>     (sreq_)->comm = comm;                                     \
>     (sreq_)->cc                          = 1;                         \
>     (sreq_)->cc_ptr              = &(sreq_)->cc;              \
> +    (sreq_)->partner_request   = NULL;                          \
>     MPIR_Comm_add_ref(comm);                                  \
>     (sreq_)->status.MPI_ERROR    = MPI_SUCCESS;               \
>     (sreq_)->status.cancelled    = FALSE;                     \
> @@ -331,6 +332,8 @@
>     (sreq_)->dev.segment_ptr     = NULL;                      \
>     (sreq_)->dev.OnDataAvail     = NULL;                      \
>     (sreq_)->dev.OnFinal         = NULL;                      \
> +    (sreq_)->dev.iov_count      = NULL;                      \
> +    (sreq_)->dev.iov_offset     = NULL;                      \
> }
> /* This is the receive request version of MPIDI_Request_create_sreq */
> @@ -353,11 +356,14 @@
>     (rreq_)->cc_ptr              = &(rreq_)->cc;              \
>     (rreq_)->status.MPI_ERROR    = MPI_SUCCESS;               \
>     (rreq_)->status.cancelled    = FALSE;                     \
> +    (rreq_)->partner_request   = NULL;                          \
>     (rreq_)->dev.state = 0;                                     \
>     (rreq_)->dev.cancel_pending = FALSE;                        \
>     (rreq_)->dev.datatype_ptr = NULL;                           \
>     (rreq_)->dev.segment_ptr = NULL;                            \
>     (rreq_)->dev.iov_offset   = 0;                              \
> +    (rreq_)->dev.OnDataAvail    = NULL;                      \
> +    (rreq_)->dev.OnFinal        = NULL;                      \
>      MPIDI_CH3_REQUEST_INIT(rreq_);\
> }

William Gropp
Deputy Director for Research
Institute for Advanced Computing Applications and Technologies
Paul and Cynthia Saylor Professor of Computer Science
University of Illinois Urbana-Champaign

Change History (12)

comment:1 Changed 10 years ago by William Gropp

  • id set to 288

This message has 0 attachment(s)

comment:2 Changed 10 years ago by goodell

Well, for starters there's no clear statement anywhere which fields were chosen to be pre-initialized and which ones were to be lazy-initialized. Examining these two (subtly different) macros for fields that are initialized and then computing the complement from the universe of MPID_Request fields isn't something that I can do easily or quickly in my head. If you give me a choice between high-performance fragile code and medium performance robust code, I'll take the robust code.

Tracking the entire life cycle of some of these requests is pretty difficult. In this case I'm fixing problems in the nemesis LMT implementation, so that's probably where the initialization should occur.


comment:3 Changed 9 years ago by balaji

  • Milestone set to mpich2-1.1.1
  • Owner set to goodell
  • Summary changed from Re: [mpi-all-commits] r3512 - mpich2/branches/dev/knem/src/mpid/ch3/include to Request initialization

comment:4 Changed 9 years ago by goodell

  • Milestone changed from mpich2-1.1.1 to mpich2-1.1.2
  • Priority changed from major to minor

Cleaning up our request creation/management functionality is unlikely to happen for the impending 1.1.1 release. Moving to 1.1.2.

Some random notes off the top of my head so that I don't forget them:

  • We need to setup timing calipers around these macros so we can accurately and scientifically assess the impact of request creation.
  • It would be nice to convert these macros to inline functions. Debugging memory issues is more difficult with both gdb and valgrind when using macros.
  • Making a list of all of the places where we currently create requests with any special notes about those places would be useful. Then we can make a list of all the fields in the structure and figure out which need to be initialized always and which do not. Then this ought to be documented in the code in comments, not in a separate document so that we have a half a shot at actually keeping the docs up to date.
  • If request creation is sufficiently expensive and lies on the critical path, then we might want to consider pre-allocation with pre-initialization in order to shave the latency a bit (we already pre-allocate via the custom handle allocators, IIRC). This might make it possible to initialize all fields to sensible values and not pay for the cost in the critical path most of the time.
  • On some platforms we might be able to zero most/all of the fields faster with non-temporal writes to memory, which should have a decent impact on the performance of these macros since most of the initializations are to zero.

comment:5 Changed 9 years ago by balaji

  • Milestone changed from mpich2-1.1.2 to mpich2-1.2

Milestone mpich2-1.1.2 deleted

comment:6 Changed 9 years ago by goodell

  • Description modified (diff)
  • Milestone changed from mpich2-1.2 to mpich2-1.2.1
  • Status changed from new to accepted

I still very much want to do this, but it's not going to happen for the upcoming release.

comment:7 Changed 9 years ago by goodell

  • Description modified (diff)
  • Milestone changed from mpich2-1.2.1 to mpich2-1.3

comment:8 Changed 8 years ago by thakur

  • Milestone changed from mpich2-1.3 to mpich2-1.4

comment:9 Changed 7 years ago by balaji

  • Milestone changed from mpich2-1.4 to mpich2-1.5

comment:10 Changed 7 years ago by goodell

  • Milestone changed from mpich2-1.5 to future

comment:11 Changed 5 years ago by balaji

  • Description modified (diff)
  • Status changed from accepted to new

comment:12 Changed 5 years ago by balaji

  • Owner goodell deleted
Note: See TracTickets for help on using tickets.