Skip to content

Performance #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
taokayan wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Performance #30

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9550b87
performance improve by move old buffer to backup instead of allocatin…
taokayan Nov 28, 2018
711cb5b
fix compile error
taokayan Nov 29, 2018
8c9b02b
Merge branch 'master' into performance
taokayan Nov 29, 2018
5f04cbe
fix modify, add replace, use move-copy as possibble
taokayan Nov 30, 2018
890a947
fix replace
taokayan Dec 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions include/chainbase/chainbase.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,14 @@ namespace chainbase {
}

template<typename Modifier>
void modify( const value_type& obj, Modifier&& m ) {
on_modify( obj );
void modify( const value_type& obj, Modifier&& m, bool move_old_value = false) {
on_modify( obj, move_old_value );
auto ok = _indices.modify( _indices.iterator_to( obj ), m );
if( !ok ) BOOST_THROW_EXCEPTION( std::logic_error( "Could not modify object, most likely a uniqueness constraint was violated" ) );
}

void remove( const value_type& obj ) {
on_remove( obj );
void remove( const value_type& obj, bool move_old_value = false ) {
on_remove( obj, move_old_value );
_indices.erase( _indices.iterator_to( obj ) );
}

Expand Down Expand Up @@ -504,7 +504,7 @@ namespace chainbase {
private:
bool enabled()const { return _stack.size(); }

void on_modify( const value_type& v ) {
void on_modify( const value_type& v, bool move_old_value ) {
if( !enabled() ) return;

auto& head = _stack.back();
Expand All @@ -516,10 +516,14 @@ namespace chainbase {
if( itr != head.old_values.end() )
return;

head.old_values.emplace( std::pair< typename value_type::id_type, const value_type& >( v.id, v ) );
if (move_old_value) {
head.old_values.emplace( std::pair< typename value_type::id_type, value_type >( v.id, std::move(const_cast<value_type&>(v))));
} else {
head.old_values.emplace( std::pair< typename value_type::id_type, const value_type& >( v.id, v ) );
}
}

void on_remove( const value_type& v ) {
void on_remove( const value_type& v, bool move_old_value ) {
if( !enabled() ) return;

auto& head = _stack.back();
Expand All @@ -538,7 +542,11 @@ namespace chainbase {
if( head.removed_values.count( v.id ) )
return;

head.removed_values.emplace( std::pair< typename value_type::id_type, const value_type& >( v.id, v ) );
if (move_old_value) {
head.removed_values.emplace( std::pair< typename value_type::id_type, value_type >( v.id, std::move(const_cast<value_type&>(v))));
} else {
head.removed_values.emplace( std::pair< typename value_type::id_type, const value_type& >( v.id, v ) );
}
}

void on_create( const value_type& v ) {
Expand Down Expand Up @@ -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 )
Copy link
Copy Markdown

@wanderingbort wanderingbort Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@taokayan taokayan Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace() added, modify() is now the original behavior.

{
CHAINBASE_REQUIRE_WRITE_LOCK("modify", ObjectType);
typedef typename get_index_type<ObjectType>::type index_type;
get_mutable_index<index_type>().modify( obj, m );
get_mutable_index<index_type>().modify( obj, m, move_old_value);
}

template<typename ObjectType>
void remove( const ObjectType& obj )
void remove( const ObjectType& obj, bool move_old_value = false )
Copy link
Copy Markdown

@wanderingbort wanderingbort Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

@wanderingbort wanderingbort Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same goes for the other ::remove method above (which eventually calls the same ::on_remove method)

Copy link
Copy Markdown
Author

@taokayan taokayan Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
CHAINBASE_REQUIRE_WRITE_LOCK("remove", ObjectType);
typedef typename get_index_type<ObjectType>::type index_type;
return get_mutable_index<index_type>().remove( obj );
return get_mutable_index<index_type>().remove( obj, move_old_value );
}

template<typename ObjectType, typename Constructor>
Expand Down