Improve adjust_balance performance #1083 #1099

abitmore · 2018-06-26T20:10:41Z

PR for #1083.

In this PR we added a new compile-time option: ASSET_BALANCE_SORTING, enabled by default.

when the option is enabled, witness_node will effectively keep the old behavior, which means nothing will change;
when the option is disabled,
- witness_node will have better performance:
  - total replay time for 27,000,000 blocks reduced by 1 - 5063/5633 ~= 10%. More profiling data is here: Improve adjust_balance performance #1083 (comment)
- A side effect is that asset_api::get_asset_holders(...) and asset_api::get_all_asset_holders() will return unordered results.

To disable the new compile-time option (before compiling):

cmake -DCMAKE_BUILD_TYPE=Release -DASSET_BALANCE_SORTING=OFF ..

Note: asset_api::get_asset_holders(...) and asset_api::get_all_asset_holders() will return unordered results after this change.

abitmore · 2018-06-27T00:22:05Z

I'm thinking about adding a compile-time option, e.g. name it ENABLE_ASSET_BALANCE_SORTING, disabled by default for better performance. API nodes that want to enable asset::api can be compiled with that option enabled. Or enable it by default?

Looks like this approach can please both parties. Also not too hard to code (although the code would be uglier).

Thoughts?

oxarbitrage · 2018-06-27T00:48:41Z

That will work for me. Thanks for considering it. El mar., jun. 26, 2018 9:22 PM, Abit <notifications@github.com> escribió:

…

I'm thinking about adding a compile-time option, e.g. name it ENABLE_ASSET_BALANCE_SORTING, disabled by default for better performance. API nodes that want to enable asset::api can be compiled with that option enabled. Or enable it by default? Looks like this approach can please both parties. Also not too hard to code. Thoughts? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1099 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUrjaSnPKEm3dIPB3OaP4mg2buNkuQ4Jks5uAtAxgaJpZM4U4mTD> .

abitmore · 2018-06-28T19:47:57Z

Added new compile-option ASSET_BALANCE_SORTING.

OP updated accordingly.

abitmore · 2018-07-14T09:31:18Z

Changed the new option to ON by default to keep old behavior, which would be more convenient for Docker users, API nodes and ignorant node operators. It's easier to contact witnesses and seed node operators and suggest them to disable it. OP updated.

jmjatlanta · 2018-07-16T10:52:16Z

libraries/chain/account_evaluator.cpp

+      // From next maintenance interval, need to stop updating this account's active/owner.
+      // Here we check if each asset is added earlier in this maintenance interval,
+      //   if yes, remove it from "added"; otherwise, add it to "removed".
+      d.modify( special_meta, [&assets_before](special_assets_meta_object& p) {


Here we are modifying a persisted data object. Could this cause problems if you run witness_node with ASSET_BALANCE_SORTED and later without (or vice-versa, or yes-no-yes, or no-yes-no)? I have not tested these situations, just typing out loud.

If it does cause problems, it seems the use of this feature will be rare. So are the symptoms easily detectable? If so, we could [log a message | force a replay]. We should also document the consequences.

Good questions.

when to replay

Currently only DB_VERSION is used to detect whether need a replay.

Ideally the node should automatically detect a) whether db schema changed E.G. caused by code changes or compile-time option changes and b) whether certain options in config.ini changed. But probably need too much work to make it perfect.

For this issue, a quick solution is to define DB_VERSION to different values when using different compile-time options.

automatically replay or not

The main concern is that replay takes long time. In case when a node operator accidentally ran a node with an incompatible data directory, being forced to replay is painful. So it makes some sense if the node refuses to start and shows an error message when incompatible db schema is detected.

IMHO these questions worth a new issue, or perhaps two issues.

@pmconrad @oxarbitrage your opinion?

For this issue, a quick solution is to define DB_VERSION to different values when using different compile-time options.

Done.

IMO the quick solution is good enough.

oxarbitrage · 2018-07-22T01:45:22Z

please note that each time the flag is changed you need to run cmake again with the flag on or off. then at make the entire project will be compiled thus making the process of testing/developing painful.

mentioning this as i think it is not a solution to abuse.

abitmore · 2018-07-22T09:33:41Z

Usually I use two build directories or more to test different values. Don't know if there is a better way.

jmjatlanta

Small fix to CMakeLists.txt

jmjatlanta · 2018-07-25T13:49:31Z

CMakeLists.txt

@@ -69,6 +69,13 @@ IF(NOT "${Boost_VERSION}" MATCHES "1.53(.*)")
   SET(Boost_LIBRARIES ${BOOST_LIBRARIES_TEMP} ${Boost_LIBRARIES})
 ENDIF()

+OPTION( ASSET_BALANCE_SORTING "Keep account_balance objects sorted by asset and amount, to be used in asset_api (ON OR OFF)" ON )


All code other than CMAKE uses ASSET_BALANCE_SORTED, but CMAKE is looking for ASSET_BALANCE_SORTING. The easier fix is to change it in CMakeLists.txt.

My opinion: ASSET_BALANCE_SORTING is not as descriptive as ASSET_BALANCE_SORTED. ASSET_BALANCE_SORTING makes me think it is not an ON/OFF option, but perhaps an algorithm choice.

Ahhh, Now I see why that was done. CMake doesn't like it when their variables are the same name as compile variables. I think there is a workaround for more recent versions of cmake, but I haven't explored it. I'm cancelling my request for changes.

Or change the CMAKE option to something else, e.g. SORT_ASSET_BALANCE (verb+object)? To be honest I'm not good at naming things. Actually, when the option is ON, we sort balances by asset + quantity; when it's OFF, we sort balances by asset only.

And I do not like haggling about variable names. Had we needed to chose one or the other, I was stating my opinion. But now we don't need to choose. I think your name is clear enough, especially with the option comment next to it.

Wrong assumption

add asset api test case

jmjatlanta

All looks good. I tested this along with the test created by @oxarbitrage in PR #1169 on Ubuntu 18.04, with ASSET_BALANCE_SORTING ON and OFF.

I am marking this approved, but perhaps merging PR 1169 into this branch before merging this into develop would be a good thing.

What should be the procedure in this case? Should I (a) "Request Changes" until 1169 is merged in here? Or (b) mark this "Approved" and keep 1169 independent?

Update: Merge happened while I was writing review. Thanks!

pmconrad

I think it might be a good idea (and simplify the code) if you switch the special authority logic to the separate account_special_balance index unconditionally, and have the ASSET_BALANCE_SORTED flag only affect the sorting of asset balances.

The great advantage I see there is that the chain logic does not depend on a compile-time flag.

pmconrad · 2018-07-25T15:54:12Z

libraries/chain/db_balance.cpp

-         b.balance = delta.amount.value;
-         if( b.asset_type == asset_id_type() ) // CORE asset
+         b.balance = delta.amount;
+         if( maint_flag )
            b.maintenance_flag = true;


Use unconditional set: b.maintenance_flag = maint_flag;

pmconrad · 2018-07-25T16:26:18Z

libraries/chain/account_object.cpp

+flat_set<asset_id_type> account_object::get_top_n_control_assets() const
+{
+   flat_set<asset_id_type> result;
+   if( owner_special_authority.which() == special_authority::tag< top_holders_special_authority >::value )


visitor pattern might look better here

pmconrad · 2018-07-25T16:56:56Z

libraries/chain/account_evaluator.cpp

+               p.special_assets_added_this_interval.insert( a );
+         }
+      });
+#endif
   }

   return void_result();


IMO the handling of special_assets_meta_object is broken in several respects:

account_create_evaluator::do_apply can create special authorities, similar code needs to be added there as well

you don't handle the case if an account switches from one asset to another (i. e. both sa_before and sa_after are true)

you cannot remove an asset just because an account doesn't use it anymore - it could still be used by a different account

Please add unit tests for all of these.

Thanks. My bad. Apparently the code is incomplete. I don't know why I didn't realize it. Perhaps I was thinking that it's updating an asset but not an account.

incomplete code

abitmore · 2018-07-25T21:13:13Z

Still need much work to get this PR done, and it's possible that we won't merge it at all. I've removed it from this release, and will rebase other PR's which were based on it to be based on develop branch, hope that they can get merged for the coming release.

I think it might be a good idea (and simplify the code) if you switch the special authority logic to the separate account_special_balance index unconditionally, and have the ASSET_BALANCE_SORTED flag only affect the sorting of asset balances.

The great advantage I see there is that the chain logic does not depend on a compile-time flag.

If we switch the special authority logic to the separate account_special_balance index unconditionally, it will impact performance of nodes which have enabled the compile-time option, due to additional data copy and modification.

Actually, as mentioned in the in-code comments, if there are too many accounts with special authorities, or the configured assets have too many holders (E.G. BTS), the approach done in this PR (massive data copy on maintenance interval) can greatly impact performance, which can be considered as an attack factor.

I guess I'll abandon the whole idea and close this PR. Some code may still be usable, so I will cherry-pick them to a new branch and submit a new PR.

Hope that we can find a better solution.

Improve adjust_balance performance #1083

3a654f2

Note: asset_api::get_asset_holders(...) and asset_api::get_all_asset_holders() will return unordered results after this change.

abitmore added this to the 201807 - Next Non-Consensus-Changing Release milestone Jun 26, 2018

oxarbitrage mentioned this pull request Jun 27, 2018

add get_asset_markets #1101

Open

17 tasks

Add compile-time option ASSET_BALANCE_SORTING

26e6474

abitmore mentioned this pull request Jul 1, 2018

Improve performance of database::update_expired_feeds() #1093

Closed

4 tasks

abitmore mentioned this pull request Jul 14, 2018

add startup option to update votes for inactive objects in api nodes #987

Closed

17 tasks

Enable ASSET_BALANCE_SORTING by default

d5c35c5

CMakeList.txt minor update

d11b75e

jmjatlanta reviewed Jul 16, 2018

View reviewed changes

abitmore force-pushed the 1083-adjust-balance branch from d6712c2 to 0ba0e30 Compare July 16, 2018 22:53

Change DB_VERSION if ASSET_BALANCE_SORTING is OFF

42de5b9

abitmore force-pushed the 1083-adjust-balance branch from 0ba0e30 to 42de5b9 Compare July 16, 2018 22:54

Minor config.hpp update

2945e1f

abitmore requested review from jmjatlanta and pmconrad July 21, 2018 17:21

add asset api test case

a10ec85

abitmore mentioned this pull request Jul 22, 2018

Ability to airdrop/sharedrop/pay dividends of one asset to all holders of another. #1172

Open

6 tasks

oxarbitrage added 2 commits July 23, 2018 10:44

move to new asset_api_tests file

0b370cf

remove non needed includes

d9aecc6

abitmore mentioned this pull request Jul 23, 2018

[7.5][TBD] Better node management bitshares/bitshares-ui#1521

Closed

oxarbitrage mentioned this pull request Jul 25, 2018

Add compile-time option TRACK_STANDBY_VOTES #987 #1140

Closed

jmjatlanta mentioned this pull request Jul 25, 2018

add asset api test case #1169

Merged

jmjatlanta previously requested changes Jul 25, 2018

View reviewed changes

Merge pull request #1169 from oxarbitrage/asset_api_testcase

8c1a579

add asset api test case

jmjatlanta previously approved these changes Jul 25, 2018

View reviewed changes

pmconrad suggested changes Jul 25, 2018

View reviewed changes

abitmore modified the milestones: 201807 - Next Non-Consensus-Changing Release, Future Non-Consensus-Changing Release Jul 25, 2018

oxarbitrage mentioned this pull request Jul 29, 2018

add asset_api_tests #1202

Merged

abitmore closed this Aug 8, 2018

abitmore deleted the 1083-adjust-balance branch August 8, 2018 16:23

abitmore mentioned this pull request Sep 5, 2018

Improve adjust_balance performance #1083

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve adjust_balance performance #1083 #1099

Improve adjust_balance performance #1083 #1099

abitmore commented Jun 26, 2018 •

edited

Loading

abitmore commented Jun 27, 2018 •

edited

Loading

oxarbitrage commented Jun 27, 2018 via email

abitmore commented Jun 28, 2018

abitmore commented Jul 14, 2018

jmjatlanta Jul 16, 2018

abitmore Jul 16, 2018 •

edited

Loading

abitmore Jul 16, 2018

pmconrad Jul 17, 2018

oxarbitrage commented Jul 22, 2018

abitmore commented Jul 22, 2018

jmjatlanta left a comment

jmjatlanta Jul 25, 2018

jmjatlanta Jul 25, 2018

abitmore Jul 25, 2018

jmjatlanta Jul 25, 2018

jmjatlanta left a comment •

edited

Loading

pmconrad left a comment

pmconrad Jul 25, 2018

pmconrad Jul 25, 2018

pmconrad Jul 25, 2018

abitmore Jul 25, 2018

abitmore commented Jul 25, 2018

Improve adjust_balance performance #1083 #1099

Improve adjust_balance performance #1083 #1099

Conversation

abitmore commented Jun 26, 2018 • edited Loading

abitmore commented Jun 27, 2018 • edited Loading

oxarbitrage commented Jun 27, 2018 via email

abitmore commented Jun 28, 2018

abitmore commented Jul 14, 2018

Choose a reason for hiding this comment

abitmore Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Jul 22, 2018

abitmore commented Jul 22, 2018

jmjatlanta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmjatlanta left a comment • edited Loading

Choose a reason for hiding this comment

pmconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Jul 25, 2018

abitmore commented Jun 26, 2018 •

edited

Loading

abitmore commented Jun 27, 2018 •

edited

Loading

abitmore Jul 16, 2018 •

edited

Loading

jmjatlanta left a comment •

edited

Loading