[petsc-dev] PetscViewerASCIIPushSynchronized

[Smith, Barry F.]

   I think this thing maybe over-designed. When I wrote it I was obsessed with having a single Flush method that one calls on the viewer that handles any type of flushing, yet efficiently. Hence the trickery. If we had a separate flush method for the synch calls I think we may be able to remove the push/pop and only use the synch flush in the code when necessary. I'll give it a try.

   Barry


> On May 2, 2019, at 11:08 PM, Jed Brown <jed at jedbrown.org> wrote:
> 
> "Smith, Barry F." <bsmith at mcs.anl.gov> writes:
> 
>>> On May 2, 2019, at 6:15 PM, Jed Brown <jed at jedbrown.org> wrote:
>>> 
>>> I think this is a consequence of PetscViewerFlush_ASCII flushing any
>>> synchronized messages (code is basically copied from
>>> PetscSynchronizedFlush).
>>> 
>>> if (vascii->allowsynchronized) {
>>>   PetscMPIInt   tag,i,j,n = 0,dummy = 0;
>>>   char          *message;
>>>   MPI_Status    status;
>>> 
>>>   ierr = PetscCommDuplicate(comm,&comm,&tag);CHKERRQ(ierr);
>>> 
>>>   /* First processor waits for messages from all other processors */
>>>   if (!rank) {
>>> 
>>> 
>>> But if all ranks don't agree about whether synchronized messages may
>>> exist, then this slow code path would always be taken.  In the current
>>> code, PetscViewerASCIIPopSynchronized doesn't do anything other than
>>> tracking that the pushes were matched.  It should probably flush
>> 
>>   What about the push? Does it need to do a flush also?  Note that the queued up messages are not tagged by the synchronized count so with 
>> multiple pushes there can be jumbled mess. 
> 
> I would think printing in the order of the pops would be okay.  It means
> each push would use a fresh buffer so only the content printed inside
> that level of nesting would print.  SegBuffer would be convenient for
> this buffer management.
> 
>>> (or
>>> provide equivalent semantic on ordering).
>> 
>> Maybe the whole chunk of code
>> 
>> if (vascii->allowsynchronized) {
>>   PetscMPIInt   tag,i,j,n = 0,dummy = 0;
>>    ....
>> 
>> could just be moved to the Pop. Or even better if possible have it call PetscSynchronizedFlush()  and
>> avoid the duplicate code. I'm not sure why I duplicated the code  but there must have been a reason.
> 
> Yup, but it should be possible to deduplicate, even if via an internal helper.
> 
>> The current code is a bit defective. Maybe the push/pop model should be abandoned and instead require that Allow be closed before another one is open. So PetscViewerASCIIOpenSynchronized(), synchronized writes, PetscViewerASCIICloseSynchronized() and the close automatically flushes. I'm not sure if the current routines are used in a nested manner (nor how to search for that ;)).
> 
> Insert a check for allowsynchronized > 1 and run the test suite.
> 
>> After a few hours I'm becoming more convinced that the reason for the Allow/Push/Pop model is just to avoid the global synchronization.
> 
> I suspect that's right.