move constructors likely a requirement

[Rik]

jwe, Dan,

I looked closely at the 9 instances of the ~octave_value_list destructor
being called for the simple expression "sin (1);".  I think I understand
the problematic coding pattern, but I haven't yet found a solution.

For didactic purposes, assume there is a complicated class (comp_class)
which has an extensive constructor/destructor or otherwise uses a lot of
resources.  There is also a function main which uses an instance of
comp_class, but the initialization of the instance is done by a separate
function init to make the code more modular and readable.

main ()
{
  comp_class cobj;

  cobj = init ();

  ...
}

comp_class init (void)
{
  comp_class retval;  // local variable

  retval.xxx = yyy;   // initialization
  ...

  return retval;      // return by value (a comp_class object)
}

The sequence of events for the C++ runtime is

1) local variable cobj is created in main calling comp_class constructor
2) init() is called
3) local variable retval is created in main calling comp_class constructor
4) "return retval;" statement causes comp_class copy constructor to execute
copying local variable retval in to a temporary new comp_class object
because the function init returns by value.
5) init() routine completes, local variable retval goes out of scope and
comp_class destructor is called
6) back in main(), destructor for cobj is called to clear object before
assignment.
7) copy assignment operator transfers temporary comp_class object to cobj.
8) temporary value goes out of scope and calls comp_class destructor

That is a huge number of objects created/destroyed to handle what is a
fairly simple coding pattern.  This example is not dreamt up, but reflects
the coding in pt-eval.cc.  The function
tree_evaluator::visit_index_expression is coded like so

octave_value_list first_args;
...
first_args = convert_to_const_vector (al);

where convert_to_const_vector is declared as

octave_value_list
tree_evaluator::convert_to_const_vector (tree_argument_list *arg_list,
                                           const octave_value *object)

One obvious solution would be to discard returning by value.  In that case,
perhaps passing a reference to the octave_value_list in to the child
function so that it operates directly on the only instance we care to
create.  But, this would mean changing a fair number of APIs and
potentially a lot of code refactoring.

The next most obvious thing would be to use the C++11 feature of move
constructors and move assignment operators.  I added those two functions to
ovl.h along with some logging.  Now when I run I get

sin(1);
octave_value_list: move constructor 893
~octave_value_list: 7772
~octave_value_list: 7773
octave_value_list: move assignment 3178
~octave_value_list: 7774
octave_value_list: move assignment 3179
~octave_value_list: 7775
octave_value_list: move assignment 3180
~octave_value_list: 7776
octave_value_1111111111list: move assignment 3181
~octave_value_list: 7777
~octave_value_list: 7778
~octave_value_list: 7779
octave_value_list: move assignment 3182
~octave_value_list: 7780

Clearly, the move routines themselves are getting called by the C++ runtime
in both flavors (constructor and assignment).  However, there is no
decrease in the number of times (9) the ~octave_value_destructor is called
because I couldn't find a clean way to "zero out" the rvalue object that is
being moved.  Here is the assignment routine I have at the moment:

  octave_value_list& operator = (octave_value_list&& obj)
  {
    static long int n = 0;
    std::cerr << "octave_value_list: move assignment " << ++n << std::endl;

    if (this == &obj)
      return *this;

    std::swap (m_data, obj.m_data);
    std::swap (m_names, obj.m_names);

    return *this;
  }

Maybe this is actually okay, but it doesn't actually result in any
speed-up.  It seems to me that the octave_value class may also need to have
move functions written at the same time.  An octave_value is also a
"complicated class" and there are many functions which return by value an
octave_value instance.  Given that this requires creating a temporary, and
the atomic increment/decrement operators are slow, move routines in that
class may be more important.

--Rik