This is turning into a nightmare. The current code is buggy and all the tests work presumably because the source was hacked just enough for them to behave properly. A big problem is PetscViewerGetSubViewer() which claims to be collective but then is used non-collectively in the source, handling the output from the sub viewers properly if it is not used collectively is impossible, but using it collectively seems to imply at least a global reduction for each PetscViewerRestoreSubViewer() to properly flush material printed in the sub viewers. I'm debating what, if anything, to do on this topic.
> On May 3, 2019, at 4:36 PM, Hapla Vaclav <[email protected]> wrote: > > > >> On 3 May 2019, at 17:34, Smith, Barry F. <[email protected]> wrote: >> >> >> 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. > > Sounds good. It would reflect PetscSynchronizedPrintf/Flush behavior which I > think is an advantage for user's understanding. > > BTW Perhaps > PetscSynchronizedPrintf(comm > could then become just a shorthand to > PettscViewerASCIISynchronizedPrintf(PETSC_VIEWER_STDOUT_(comm) > The same for SynchronizedFlush. > > Vaclav > >> >> Barry >> >> >>> On May 2, 2019, at 11:08 PM, Jed Brown <[email protected]> wrote: >>> >>> "Smith, Barry F." <[email protected]> writes: >>> >>>>> On May 2, 2019, at 6:15 PM, Jed Brown <[email protected]> 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. >> >
