DOLFIN

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

fast-array
Merge into trunk

Revision history for this message

Anders Logg (logg) wrote on 2012-03-06:

On Tue, Mar 06, 2012 at 12:28:20PM -0000, Garth Wells wrote:
> On 6 March 2012 12:11, Anders Logg <email address hidden> wrote:
> > I'm also very much in favor.
> >
>
> I'm happy in principle, but want some clarity on Array. Can we make it
> that Array is always a view (i.e., never owns that data), and
> therefore cannot be resized? This would clean up some const hacks.

That's fine with me. And in addition we should limit the use of Array
to just the few places where we need it for passing data through
the SWIG layer without copying.

--
Anders

> Garth
>
> >
> > 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
> >>
> >>
> >
>

On Tue, Mar 06, 2012 at 12:28:20PM -0000, Garth Wells wrote:
> On 6 March 2012 12:11, Anders Logg <logg@simula.no> wrote:
> > I'm also very much in favor.
> >
>
> I'm happy in principle, but want some clarity on Array. Can we make it
> that Array is always a view (i.e., never owns that data), and
> therefore cannot be resized? This would clean up some const hacks.

That's fine with me. And in addition we should limit the use of Array
to just the few places where we need it for passing data through
the SWIG layer without copying.

--
Anders

> Garth
>
> >
> > 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 <jobh@simula.no> 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