lmi
[Top][All Lists]
Advanced

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

[lmi] Confusion about any_member<> assignment operator


From: Vadim Zeitlin
Subject: [lmi] Confusion about any_member<> assignment operator
Date: Thu, 19 Feb 2015 01:13:11 +0100

 Hello,

 In my search for better performance of the census view I reached the point
where the bottleneck is the access of the elements of MemberSymbolTable
(why is it so and the preceding steps will be explained in details in a
separate message, here I just wanted to give some context as to why I was
looking into this at all). Because of this I looked into replacing the
generic std::map used by this class currently with something more suitable
for the initialize-once-access-many-times pattern corresponding to its use.
The well-known best choice for this case is a sorted array, e.g. such as
used by boost::container::flat_map, so I experimentally tried using it.

 Unfortunately this didn't work at all because of assertion failures in
holder<>::assign() when it was called from any_member::operator=() during
the (inevitable, with the current MemberSymbolTable interface) vector
reallocations because it attempted to copy a holder of one type to another.
Considering that holder<> is a template parametrized by type, it indeed
clearly doesn't make sense to do this, so the only solution is to not call
assign() in this case.

 IOW I believe the problem is really in any_member::operator=() as, in
spite of the following comment in any_member.hpp before any_member:

// This class is necessarily Assignable, so that a std::map can hold it.

This class is clearly not Assignable as only some of the objects of this
class can be assigned to each other, but not all of them. So my solution
was to reimplement any_member::operator=() by recreating the contents_
pointer because, I thought, any_member objects were pointer-like and so
assignments to them should reset the pointer itself and not the value of
whatever it points to. And while "Assignable" is not actually a requirement
for the map elements, this change did make the class satisfy it which
seemed to be a good thing. And, of course, this also fixed the asserts
about the mismatching types in holder<>::assign() as it wasn't called at
all any more.

 However it also completely broke the program. This is because
MemberSymbolTable<>::assign() uses any_member<>::operator=() and relies on
it to update the value of the object pointed to and not just the pointer
itself. Getting a bit desperate to make my flat_map-based approach to work
(it was just an experiment, after all, so I didn't mean to spend half a day
on it as I ended up doing), I kept the original assignment operator as
assign(any_member const&) method and added a new assignment operator which
does the right thing from the "Assignable" requirement point of view.

 The good thing is that this does seem to work, i.e. the program seems to
behave correctly in interactive tests and the unit tests pass too. Another
good thing is that it does result in the expected gain in performance
compared to std::map (26% is not bad, although less impressive when
compared to C++11 std::unordered_map against which we get 3% only...).
The bad thing is that we now have two operator[] overloads in any_member
with very different semantics: the one taking any_member resets the
pointer, while the one taking std::string changes the value of the object
being pointed to. And the ugly thing is that I absolutely don't know any
more what is right and what is wrong and am thoroughly confused.

 So I'd like to ask for your help here. Is any_member<> supposed to be
value-like or pointer-like? Forgetting about the optimizations which led me
to this originally, I think it's important to clear up the confusion with
it being (or not) "Assignable": right now it definitely isn't, and the
simplest solution is, of course, to just remove the comment saying that it
is, but having an operator[] which doesn't accept all objects is pretty
unusual in not a good way too IMO.

 Sorry for bothering you and I'd understand if you didn't want to dive into
this, but I think I have exactly 0 chances of coming up with a patch
acceptable to you without at least some guidance from your part.

 I won't have time to work on this at least until Monday, but if you have
any questions about the story so far, please just ask and I'll do my best
to answer them a.s.a.p.

 Thanks in advance for your help!
VZ

reply via email to

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