Code review comment for lp:~jobh/dolfin/symmetric-assemble

On Thu, Feb 02, 2012 at 10:20:13AM -0000, Garth Wells wrote:
> On 2 February 2012 10:01, Joachim Haga <email address hidden> wrote:
> >>> Not sure about multicore, but can have a look.
> >>
> >> This is similar to the present AssembleSystem, in that it essentially iterates
> >> over cells and then facets in each cell.
> >>
> >> AFAIK, assemble over interior faces have not got as much love as the other
> >> integrals. There are also some code dublications which can be removed (I
> >> think). We have assemble_cells and assemble_cells_and_exterior_facets. One can
> >> probably just have the latter.
> >
> > Ok. I assume there's a reason that multicore does it in this way,
> > meaning that if they are combined then it's the multicore version that
> > "wins". Other than that, I guess it's just a matter of specifying a
> > single thread and (if necessary) shorting out the mesh coloring in the
> > single-thread case.
> >
> > But OpenMPAssembler hasn't replaced Assembler, so I guess there are
> > problems with this approach. Performance?
> >
>
> I don't believe that it's possible to have one Assembler without
> compromising on performance.

Perhaps not, but it should be possible to share much more of the code.

> OpenMPAssembler is slower than Assembler for one thread because it
> requires a somewhat different loop over cells. Also, at least when I
> last worked on OpenMPAssembler, it didn't support as many cases as
> Assembler. Johan H has probably bridged most/all of the gap in the
> mean time.
>
> OpenMPAssembler needs more testing before removing the 'experimental' tag.

Agree.

> I think that the performance focus should be on SystemAssembler (with
> the possibility of just assembling the LHS or RHS). Assembler and
> OpenMPAssembler could be merged for now. The assembler code would be
> simpler if a number of the 'if' statements could be removed. Perhaps
> the sub-domains code should be moved to the domain (i.e., the Mesh),
> and the assemblers can just loop over sub-domains.

Yes, it should be possible to move that logic elsewhere. It probably
should not go into the mesh, since it is possible to assemble with the
subdomain specification separate from the mesh (which is the reason
for the somewhat complex logic). It might be possible to just move it
to a separate utility function, or a new data structure may be needed.

--
Anders