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 <j...@jedbrown.org> wrote:
>
> "Smith, Barry F." <bsm...@mcs.anl.gov> writes:
>
>>> On May 2, 2019, at 6:15 PM, Jed Brown <j...@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.
