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

[Vaclav Slavik]

Hi,

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. I hesitate to mess with it without improving both of these:
>   Input::operator=()
>   Input::operator==()
> at the same time (not to mention the ghastly Input::magically_rectify()).
> But solving those issues looks like it'd take a lot of effort, and the
> code does work correctly AFAIK.

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.

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
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).

Am I missing something here? If I am, what is it?

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?

Regards,
Vaclav



diff --git a/input.cpp b/input.cpp
index 0b72427..a536a43 100644
--- a/input.cpp
+++ b/input.cpp
@@ -254,17 +254,7 @@ Input::Input(Input const& z)
     ,MemberSymbolTable <Input>()
 {
     AscribeMembers();
-    std::vector<std::string>::const_iterator i;
-    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();
-        }
+    assign(z);
     DoAdaptExternalities();
 }
 
@@ -274,11 +264,7 @@ Input::~Input()
 
 Input& Input::operator=(Input const& z)
 {
-    std::vector<std::string>::const_iterator i;
-    for(i = member_names().begin(); i != member_names().end(); ++i)
-        {
-        operator[](*i) = z[*i].str();
-        }
+    assign(z);
     DoAdaptExternalities();
     return *this;
 }
@@ -286,25 +272,7 @@ Input& Input::operator=(Input const& z)
 // TODO ?? Can this be put into class MemberSymbolTable?
 bool Input::operator==(Input const& z) const
 {
-    std::vector<std::string>::const_iterator i;
-    for(i = member_names().begin(); i != member_names().end(); ++i)
-        {
-// TODO ?? Provide operator!=(). Done yet?
-//        if(operator[](*i) != z[*i])
-// TODO ?? Wait--this wouldn't work, at least not yet...
-// ...it tests *identity*, not *equivalence*.
-//        if(!(operator[](*i) == z[*i]))
-//
-// TODO ?? Then why doesn't even this work?
-//        if(!(operator[](*i).str() == z[*i].str()))
-        std::string const s0 = operator[](*i).str();
-        std::string const s1 = z[*i].str();
-        if(s0 != s1)
-            {
-            return false;
-            }
-        }
-    return true;
+    return equals(z);
 }
 
 mcenum_ledger_type Input::ledger_type () const {return GleanedLedgerType_;}