Skip to content
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

Performance #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+40 −5
Open

Performance #30

Changes from 4 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
40 changes: 35 additions & 5 deletions include/chainbase/chainbase.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I see. I'll fix it.

auto ok = _indices.modify( _indices.iterator_to( obj ), m );
if( !ok ) BOOST_THROW_EXCEPTION( std::logic_error( "Could not replace object, most likely a uniqueness constraint was violated" ) );
}

void remove( const value_type& obj ) {
on_remove( obj );
_indices.erase( _indices.iterator_to( obj ) );
Expand Down Expand Up @@ -320,7 +327,7 @@ namespace chainbase {

for( auto& item : head.old_values ) {
auto ok = _indices.modify( _indices.find( item.second.id ), [&]( value_type& v ) {
v = std::move( item.second );
v = std::move( const_cast<std::remove_const_t<decltype(item.second)>& >(item.second) );
});
if( !ok ) BOOST_THROW_EXCEPTION( std::logic_error( "Could not modify object, most likely a uniqueness constraint was violated" ) );
}
Expand All @@ -332,7 +339,7 @@ namespace chainbase {
_next_id = head.old_next_id;

for( auto& item : head.removed_values ) {
bool ok = _indices.emplace( std::move( item.second ) ).second;
bool ok = _indices.emplace( std::move(const_cast<std::remove_const_t<decltype(item.second)>& >(item.second) ) ).second;
if( !ok ) BOOST_THROW_EXCEPTION( std::logic_error( "Could not restore object, most likely a uniqueness constraint was violated" ) );
}

Expand Down Expand Up @@ -400,7 +407,7 @@ namespace chainbase {

// We can only be outside type A/AB (the nop path) if B is not nop, so it suffices to iterate through B's three containers.

for( const auto& item : state.old_values )
for( auto& item : state.old_values )
{
if( prev_state.new_ids.find( item.second.id ) != prev_state.new_ids.end() )
{
Expand Down Expand Up @@ -519,6 +526,21 @@ namespace chainbase {
head.old_values.emplace( std::pair< typename value_type::id_type, const value_type& >( v.id, v ) );
}

void on_replace( const value_type& v ) {
if( !enabled() ) return;

auto& head = _stack.back();

if( head.new_ids.find( v.id ) != head.new_ids.end() )
return;

auto itr = head.old_values.find( v.id );
if( itr != head.old_values.end() )
return;

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

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

Expand All @@ -538,7 +560,7 @@ 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 ) );
head.removed_values.emplace( std::pair< typename value_type::id_type, value_type >( v.id, std::move(const_cast<value_type&>(v))));
}

void on_create( const value_type& v ) {
Expand Down Expand Up @@ -933,13 +955,21 @@ namespace chainbase {
}

template<typename ObjectType, typename Modifier>
void modify( const ObjectType& obj, Modifier&& m )
void modify( const ObjectType& obj, Modifier&& m)
{
CHAINBASE_REQUIRE_WRITE_LOCK("modify", ObjectType);
typedef typename get_index_type<ObjectType>::type index_type;
get_mutable_index<index_type>().modify( obj, m );
}

template<typename ObjectType, typename Modifier>
void replace( const ObjectType& obj, Modifier&& m)
{
CHAINBASE_REQUIRE_WRITE_LOCK("replace", ObjectType);
typedef typename get_index_type<ObjectType>::type index_type;
get_mutable_index<index_type>().replace( obj, m );
}

template<typename ObjectType>
void remove( const ObjectType& obj )
{
Expand Down