[Octave-bug-tracker] [bug #56752] Performance slowdown from version 3.2.4 through to current dev branch

[Rik]

Follow-up Comment #30, bug #56752 (project octave):

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.


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?56752>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/