DOLFIN

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

fast-array
Merge into trunk

Revision history for this message

Garth Wells (garth-wells) wrote on 2012-03-06:

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.

Garth

> --
> 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
>>
>>
>
> --
> https://code.launchpad.net/~jobh/dolfin/fast-array/+merge/94467
> Your team DOLFIN Core Team is requested to review the proposed merge of lp:~jobh/dolfin/fast-array into lp:dolfin.

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.

Garth

> --
> 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 <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
>>
>>
>
> --
> https://code.launchpad.net/~jobh/dolfin/fast-array/+merge/94467
> Your team DOLFIN Core Team is requested to review the proposed merge of lp:~jobh/dolfin/fast-array into lp:dolfin.

« Back to merge proposal