Re: [lmi] Input::operator=, == and copy ctor

[Greg Chicares]

On 2012-03-15 18:31Z, Vaclav Slavik wrote:
> 
> On 2012-03-12 16:22, Greg Chicares wrote:
>> Class Input's copy ctor is probably time intensive. At a quick glance,
>> it would appear to convert numbers to strings and back again. As noted
>> inline, the solution that seemed obvious to me (a long time ago) does
>> not work.

Here's the copy-ctor code we're talking about (in HEAD for now):

    for(i = member_names().begin(); i != member_names().end(); ++i)
        {
        // This would be wrong:
        //   operator[](*i) = z[*i];
        // because it would swap in a copy of z's *members*.
        //
        // TODO ?? Would we be better off without the operator=() that
        // does that? Using str() here, passim, seems distateful.
        operator[](*i) = z[*i].str();
        }

Those comments resemble these:

template<typename ClassType>
any_member<ClassType>& any_member<ClassType>::operator=
    (any_member<ClassType> const& other
    )
{
    // This would be wrong:
//    any_member<ClassType>(other).swap(*this);
    // because it would swap the ClassType* object, bizarrely placing
    // a pointer to a member of one object into another object's
    // symbol table.
    LMI_ASSERT(other.content_);
    LMI_ASSERT(content_);
    content_->assign(*other.content_);
    return *this;
}

and indeed six years ago any_member<T>::operator=() simply called swap():
  
http://svn.savannah.nongnu.org/viewvc/lmi/trunk/any_member.hpp?root=lmi&r1=1593&r2=1758
but that problem was fixed; details are in the 'skeleton' trunk:
  
http://svn.savannah.nongnu.org/viewvc/skeleton/trunk/any_member.hpp?root=lmi&r1=1723&r2=1605
where Input::Input(Input const&) was then written to delegate to assign():
  
http://svn.savannah.nongnu.org/viewvc/skeleton/trunk/input.cpp?root=lmi&r1=1707&r2=1724&pathrev=1734
which is what your patch does.

> I had a look at the copy ctor under Valgrind, on a large census file
> with 236 particular cells, and indeed, over 50% of time was spent in
> string conversion.

I modified 'input_test.cpp' to measure the speed of the copy ctor,
copy assignment operator, and equality operator (though I haven't
committed that change yet). Here are my measurements, which I
tabulated by running three times and taking the median timing for
each individual line:

HEAD:
  Class 'Input' speed tests...
  Copy ctor: 3.770e-03 s =    3769589 ns, mean of 100 iterations
  Assign   : 2.120e-03 s =    2119840 ns, mean of 100 iterations
  Equals   : 1.932e-03 s =    1931836 ns, mean of 100 iterations
  Overhead : 1.892e-06 s =       1892 ns, mean of 5286 iterations
  Read     : 2.240e-03 s =    2240408 ns, mean of 100 iterations
  Write    : 1.623e-03 s =    1622649 ns, mean of 100 iterations
  'cns' io : 2.520e-02 s =   25199619 ns, mean of  40 iterations
  'ill' io : 5.767e-03 s =    5766936 ns, mean of 100 iterations

With your patch applied:
  Class 'Input' speed tests...
  Copy ctor: 1.656e-03 s =    1656023 ns, mean of 100 iterations
  Assign   : 1.752e-04 s =     175213 ns, mean of 100 iterations
  Equals   : 1.544e-04 s =     154397 ns, mean of 100 iterations
  Overhead : 1.901e-06 s =       1901 ns, mean of 5261 iterations
  Read     : 2.013e-03 s =    2013497 ns, mean of 100 iterations
  Write    : 1.482e-03 s =    1482193 ns, mean of 100 iterations
  'cns' io : 1.748e-02 s =   17478970 ns, mean of  58 iterations
  'ill' io : 5.254e-03 s =    5254161 ns, mean of 100 iterations

Those measurements are consistent with your findings, and show
that assigning a number to a number is an order of magnitude
faster than converting number --> string --> number.

> The patch below improves the performance a lot -- not using reserve() is
> still much slower with it, but the bottleneck shifts elsewhere (more on
> that later).
> 
> The patch looks scary -- it does little more than remove all the
> comments about assignment being wrong or not working. But as far as I
> can tell, it's correct. I think it used to be broken but then you fixed
> any_member<>::operator= (and ==) to behave more conventionally. Reading

Exactly--details above.

> the code, it checks held value's type and perform assignment or
> comparison on the values, not pointers. I kept reading it over and over
> and I can't find anything wrong. Of course, input_test.cpp still passes
> with my changes (and does check =, == and copy ctor).

Just to make really sure, I applied your patch, then changed
  any_member<ClassType>& any_member<ClassType>::operator=
(which is quoted in full above) to restore the "// This would be wrong:"
behavior. Then:

...without your patch, unit test 'input_test.cpp' succeeds.

...with your patch, the unit test fails, as I had hoped, in the obvious
places in the blocks bearing these comments:
    // Test copy constructor.
    // Test assignment operator.
(and then it either segfaults or reports "out of memory", but that's okay
because those effects stem from reverting to ancient and defective code).

This analysis further supports your patch, which I will soon commit.

> The other Input copy ctor performance problem is DoAdaptExternalities(),
> it recreates the (same) database on each copy. If I remove this call,
> the reserve()-less version takes only some 115% of the better version's
> time (compared to 170% originally).
> 
> This could, I think, be easily fixed by using shared_ptr for the
> database and not doing any real work in copy ctor. Should I do it?

I suspect that will be handled by the patches you posted 2012-06-15.