Code review comment for lp:~jobh/dolfin/fast-array

Revision history for this message
Anders Logg (logg) wrote :

I'm also very much in favor.

--
Anders

On Tue, Mar 06, 2012 at 11:53:31AM -0000, Johan Hake wrote:
> I am in favor of applying the patch.
>
> On Tue, Mar 6, 2012 at 12:20 PM, Joachim Haga <email address hidden> wrote:
> > This seems to have stalled without a consensus. Still, I think the improvements are too dramatic to just drop. Assembling a "v*f*dx" RHS on a 64^3 cube (twice):
> >
> > 6.95 s before
> > 2.30 s after
> >
> > With Garth's suggestion of reusing an Array: 2.54s, but not thread safe without some extra complexity (likely compiler-dependent).
> >
> > I'd like to repeat my argument that my proposed change isn't any less safe than what's there already. The old Array used reference_to_no_delete_ptr in several constructors, thus the memory was managed by the caller -- exactly the same as it is here, if the Array is not the owner. Arrays were never shared [*], so the "shared" variant is exactly the same as the new "owned" variant.
> >
> > Note [*]: It was used implicitly to transfer ownership in the TimeSeries return values. Since there's agreement than an extra copy does no harm there, I've changed those to return std::vector instead.
> >
> > Then it can be deprecated or redesigned or whatever later, but in the meantime dolfin will be much faster :)
> >
> > PS. The "interface version" thing seems to not work for all instant files, so instant-clean is still required. How should this be handled?
>
> Not sure what is not working, but it should be enough to change the
> DOLFIN_VERSION_MICRO, as this is eventualy included in the md5sum
> which determines if a modules should be recompiled or not.
>
> Johan
>
>

« Back to merge proposal