octave-maintainers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [major] struct array indexing in tip


From: Jaroslav Hajek
Subject: Re: [major] struct array indexing in tip
Date: Fri, 23 Jan 2009 13:44:53 +0100

On Wed, Jan 21, 2009 at 4:28 PM, John W. Eaton <address@hidden> wrote:
> [moved from the bug list.  --jwe]
>
> On 21-Jan-2009, Jaroslav Hajek wrote:
>
> | Okay, I've seen a couple of bugs myself, so here's another shot:
> | http://hg.savannah.gnu.org/hgweb/octave/rev/906f976d35a8
> |
> | This time, I didn't try to allow further indexing on cs-lists, like
> | a(1:2).x(1), because Matlab doesn't allow it either, and it
> | complicates things a little.
> |
> | octave:1> [a(1:2).x(2)] = deal(3,4)
> | error: a cs-list cannot be further indexed
> | error: evaluating argument list element number 1
>
> OK.
>
> | I've revamped the tree_index_expression::lvalue code to avoid useless
> | evaluation of the trailing indexing expr just for getting dimensions,
> | so things should be somewhat faster now. Also, re-evaluation of an
> | already evaluated portion of the index chain is avoided in both lvalue
> | and rvalue, so that the following code calls testfun only once:
> |
> | function y = testfun (x)
> |   disp ('called');
> |   for i = 1:5
> |     y{i} = (1:i) * x;
> |   endfor
> | endfunction
> |
> | testfun (5) {end} (end)
> |
> | instead of 3 times (prior to this patch).
>
> OK, I think this was the intent all along, but I'm not surprised that
> it was not doing the right thing...
>
> | Also, I've disallowed automatic resizing in the normal subsref
> | function, so that the following code will now produce a proper error:
> |
> | a(1).x.x = 1
> | a(2).x
>
> OK.
>
> | > Maybe someone (other than Jaroslav) would like to generate some tests
> | > so these problems don't resurface later?
> | >
> |
> | That would be great. I *may* do it eventually myself, but no promise :)
>
> We need the tests, so I'm hoping someone else will have the time to do
> it.  Anyone?
>
> | I'd still like to make more changes to the indexing code:
> |
> | First, an indexed assignment to an out-of-bounds element that does not
> | occur last in the indexing chain
> | does unnecessarily resize the array three times via the indexing with
> | resize_ok. I don't think that Array<T>::index (..., resize_ok) is
> | actually used by any instance than Cell (i.e. Array<octave_value>),
> | and probably the only correct usage is with scalar-resulting indices,
> | so I'd like to move the methods away from Array<T> to Cell and
> | optimize the all-scalars case to avoid the unnecessary resizes.
>
> OK.
>
> | Second, given that octave_value_lists are often converted to Cells and
> | vice versa, I'd like to consider rebasing octave_value_list as a Cell
> | subclass. That would allow virtually all argument-passing code benefit
> | from the shallow copy mechanism of Array<T> (now even for contiguous
> | subranges), and thus reduce the amount of copying octave_values
> | around. That would allow things like func (args{:}) to work without
> | copying.
>
> I think this is OK too, but should octave_value_list be derived from
> Cell, or contain a Cell?  Does it need to inherit all the operations
> of a Cell?  If not, I think maybe it would be better if it simply
> contained a Cell instead of a std::vector<octave_value> object.  But
> I haven't given it a lot of thought so maybe I'm missing something.
>
> jwe
>

The changesets are uploaded. octave_value_list is now implemented
using Array<octave_value>, allowing data sharing,
and shallow conversions to and from Cells. I've also dug for various
unoptimized constructions of octave_value_lists using plain loops and
replaced them by shallow copying and slicing where possible. Also,
I've added const qualifiers to several temporary variables (Cells and
octave_value_lists) that were unnecessarily forcing copies.

cf this simple benchmark (set n to a suitable value):
n = 1e6;
a = zeros (n, 2);
a = mat2cell (a, ones (n, 1), 2);
tic; [b(1:n).x] = a{:}; toc
tic; [b(1:n).y] = a{:}; toc
tic; [c{1:n}] = b.x; toc
tic; horzcat (a{:}); toc

#now fun...
function fun (varargin)
endfunction
tic; fun (c{:}); toc
tic; size_equal (a{:}); toc

with some older tip, I get:
Elapsed time is 0.772284 seconds.
Elapsed time is 0.76441 seconds.
Elapsed time is 0.25703 seconds.
Elapsed time is 1.31502 seconds.
Elapsed time is 0.249905 seconds.
Elapsed time is 0.687679 seconds.

with the recent patches, I get:
Elapsed time is 0.000152111 seconds.
Elapsed time is 6.91414e-05 seconds.
Elapsed time is 6.60419e-05 seconds.
Elapsed time is 0.899748 seconds.
Elapsed time is 0.03069 seconds.
Elapsed time is 0.191001 seconds.


In particular, you can see that cells can now be cheaply transformed
to struct components and vice versa.
The fourth case is actually realistic - there may be situations when
passing very long argument lists to a function is meaningful. The last
two benchmarks are not really serious, they just illustrate that
significantly less overhead with copying octave_values is now done.

cheers

-- 
RNDr. Jaroslav Hajek
computing expert
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz


reply via email to

[Prev in Thread] Current Thread [Next in Thread]