IBAMR / IBSAMRAI2

SAMRAI 2.4.4 fork and associated patches.
Other
1 stars 1 forks source link

avoid copying to a temporary buffer when packing ArrayData to MessageStream #10

Closed boyceg closed 1 year ago

boyceg commented 1 year ago

@drwells this seems to work with most of IBAMR's tests.

boyceg commented 1 year ago

I was having trouble with template instantiation, and instead of figuring out the right way to deal with it, I got lazy and simply moved the implementation of the specialized pack/unpack routines into ArrayData.C.

boyceg commented 1 year ago

Hmmm, may have spoken too soon --- getting a few test failures. Gotta run to (hopefully!) get my car back from the shop. I can look more later.

boyceg commented 1 year ago

Hrmmm, I think I have done something subtly wrong. 😭

boyceg commented 1 year ago

OK, these tests (for IBAMR master) fail with this branch of SAMRAI:

    adv_diff/adv_diff_02_3d.PPM.mpirun=8.input
    adv_diff/bp_adv_diff_02_2d.mpirun=8.input
    adv_diff/adv_diff_02_3d.PPM.CONSERVATIVE.mpirun=8.input
    IBFE/explicit_ex4_2d.amr.restart=50.mpirun=4.input
    IIM/flow_past_sphere.dg.mpirun=4.input
    CIB/cib_double_shell.mpirun=4.input
    IIM/poiseuille_flow.mpirun=2.input
    adv_diff/adv_diff_02_3d.mpirun=2.input
    IBFE/interpolate_velocity_01_2d.nodal_quadrature.tri6.input
    CIB/cib_double_shell.input
boyceg commented 1 year ago

Ah, well, tests fail also with our SAMRAI 2.4.4 master branch:

    adv_diff/bp_adv_diff_02_2d.mpirun=8.input
    IBFE/explicit_ex4_2d.amr.restart=50.mpirun=4.input
    IIM/flow_past_sphere.dg.mpirun=4.input
    CIB/cib_double_shell.mpirun=4.input
    IIM/poiseuille_flow.mpirun=2.input
    IBFE/interpolate_velocity_01_2d.nodal_quadrature.tri6.input
    CIB/cib_double_shell.input
drwells commented 1 year ago

Yeah, some of those are known to be unstable. This is why examples don't always make good tests (all of those are really complicated). In particular, flow_past_sphere.dg.mpirun=4.input almost passes (its just over the tolerance threshold).

drwells commented 1 year ago

I did some investigating here and it looks like there are two classes which inherit from AbstractStream:

Hence - its cleaner and we can avoid the extra dynamic_cast() if we add this as a virtual method to AbstractStream and then throw an exception if somehow someone calls it with the XDR class. We can write a follow-up in which we try to make sure the XDR class is never used (SAMRAI doesn't detect XDR support on my machines so I don't think that's going to be a significant change).

boyceg commented 1 year ago

FileStream is never used?

boyceg commented 1 year ago

Indeed, it is only used in the old SAMRAI viz output thing (Vizamrai).

boyceg commented 1 year ago

BTW, I did not add this as a virtual method here because I could not get that to play nicely with templates.

drwells commented 1 year ago

Oh yeah, that isn't going to work (template functions can't be virtual).

Let me think some more about this - perhaps there is some other way to avoid dynamic_cast.

boyceg commented 1 year ago

One way is to replace AbstractStream with MessageStream in these interfaces!

drwells commented 1 year ago

I suspect that's the right choice (since we literally never use the other ones - I don't know if they even compile any more given that SAMRAI cannot find my system XDR copy).

boyceg commented 1 year ago

I'm confused by why this made any changes at all, since it should be copying the same data in the same order . . . . .

@drwells should I go ahead and turn AbstractStream into MessageStream and remove XDRStream and FileStream?

drwells commented 1 year ago

Maybe. I'm mulling it over - an alternative might be to get rid of the template and use char *s everywhere. I'll look into it today and reach a decision.

boyceg commented 1 year ago

There is still the templating on DIM to deal with...

boyceg commented 1 year ago

I might start over from scratch on this and do it more carefully to try to get exact reproduction of the test output...

boyceg commented 1 year ago

Ah, I think I need to add an implementation of unpackStreamAndSum --- that could be the source of the diffs...

boyceg commented 1 year ago

Ah, I was forgetting to pack/unpack some of the depths. I think this is working now.

drwells commented 1 year ago

Sorry, I meant to look at this yesterday but didn't get around to it. I might need another day!

boyceg commented 1 year ago

I think that the implementation is working now, but I'm not 100% sure.

boyceg commented 1 year ago

OK, with the most recent push, we now are only getting these failures:

    adv_diff/bp_adv_diff_02_2d.mpirun=8.input
    IBFE/explicit_ex4_2d.amr.restart=50.mpirun=4.input
    CIB/cib_double_shell.mpirun=4.input
    IIM/poiseuille_flow.mpirun=2.input
    IBFE/interpolate_velocity_01_2d.nodal_quadrature.tri6.input
    CIB/cib_double_shell.input
boyceg commented 1 year ago

(Some are time-outs.)