Performance #30
Performance #30
Conversation
include/chainbase/chainbase.hpp
Outdated
| } | ||
|
|
||
| template<typename ObjectType> | ||
| void remove( const ObjectType& obj ) | ||
| void remove( const ObjectType& obj, bool move_old_value = false ) |
There was a problem hiding this comment.
I think it is always correct to std::move into the backup on remove if std::is_move_constructible<ObjectType>::value is true.
Instead of making move_old_value a parameter to the function can we instead specialize the ::on_remove method based on whether or not std::is_move_constructible<value_type>::value is true?
There was a problem hiding this comment.
same goes for the other ::remove method above (which eventually calls the same ::on_remove method)
There was a problem hiding this comment.
I found that some other functions like squash or undo already assume the value_type is move constructible. I've updated the code to use std::move on the value in on_remove().
include/chainbase/chainbase.hpp
Outdated
| @@ -933,19 +941,19 @@ namespace chainbase { | |||
| } | |||
|
|
|||
| template<typename ObjectType, typename Modifier> | |||
| void modify( const ObjectType& obj, Modifier&& m ) | |||
| void modify( const ObjectType& obj, Modifier&& m, bool move_old_value = false ) | |||
There was a problem hiding this comment.
I'm not sure I like the readability of overloading ::modify to do potentially destructive things before the modifier is invoked based on a boolean flag.
What do you think about leaving ::modify as it was (makes a copy for the backup always) and adding a method ::replace which will move into head.old_values always.
The expected ::replace method could take a "Replacer" lambda with the signature void (value_type& value_to_replace, const value_type& old_value). Like emplace it would construct the new value and pass it to the lambda which can fully initialize the replacement using the immutable old value if needed. Boost provides https://www.boost.org/doc/libs/1_67_0/libs/multi_index/doc/reference/ord_indices.html#replace which has an overload to take a rvalue. I assume this allows us to move the newly constructed replacement into the multi-index container.
There was a problem hiding this comment.
replace() added, modify() is now the original behavior.
| @@ -230,6 +230,13 @@ namespace chainbase { | |||
| if( !ok ) BOOST_THROW_EXCEPTION( std::logic_error( "Could not modify object, most likely a uniqueness constraint was violated" ) ); | |||
| } | |||
|
|
|||
| template<typename Modifier> | |||
| void replace( const value_type& obj, Modifier&& m ) { | |||
| on_replace( obj ); | |||
There was a problem hiding this comment.
This seems like a pretty dangerous way to implement this.
on_replace will std::move() from the existing object, then pass that object into the modifier. At that point the object is in a "valid by indeterminate state" so, as a caller writing the modifier lambda you have to have very deep and complete knowledge of the object you are replacing.
The reason I suggested passing a const reference to the old value, and a constructed but un-initialized non-const reference is that it forces the caller to be explicit about the operation rather than relying on implicit safety. naturally, that would mean that the "std::move" would happen after the Replacer lambda was invoked.
performance boost by move assigning old buffer to backup buffer in database modify/remove.