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.

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

[10] Performance improvement: calling set_subscribe_callback API with false #1338

Closed
abitmore opened this Issue Mar 18, 2018 · 19 comments

Comments

Projects
None yet
4 participants
@abitmore
Member

abitmore commented Mar 18, 2018

I'm not sure if this will affect reference wallet: bitshares/bitshares-core#725

/**
 * @brief Register a callback handle which then can be used to subscribe to object database changes
 * @param cb The callback handle to register
 * @param nofity_remove_create Whether subscribe to universal object creation and removal events.
 *        If this is set to true, the API server will notify all newly created objects and ID of all
 *        newly removed objects to the client, no matter whether client subscribed to the objects.
 *        By default, API servers don't allow subscribing to universal events, which can be changed
 *        on server startup.
 */
 void set_subscribe_callback( std::function<void(const variant&)> cb, bool notify_remove_create );

Currently no limitation about the second parameter. However, in next bitshares-core release, by default API servers won't allow it to be set to true.

Please check.

@abitmore abitmore added the question label Mar 18, 2018

@wmbutler wmbutler modified the milestones: 180415, 180501 Mar 18, 2018

@svk31

This comment has been minimized.

Contributor

svk31 commented Mar 19, 2018

The GUI sets this to true via the bitsharesjs ChainStore here: https://github.com/bitshares/bitsharesjs/blob/master/lib/chain/src/ChainStore.js#L163

The consequence for the GUI of being connected to a node with this boolean set to false will be that certain parts of the GUI will go out of sync with the blockchain

@abitmore abitmore changed the title from Question about use of set_subscribe_callback API to Performance improvement: calling set_subscribe_callback API with false Mar 19, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

@svk31 setting this to true causes unnecessary high traffic and load on API servers. Please subscribe to needed objects only, e.g. 2.1.0. Please investigate how much time is needed for making this change in UI, if need other API changes, please notify the back end dev team; if need too much time, we'll keep the behavior unchanged in next bitshares-core release (in 2 weeks from now).

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

For example, for the code mentioned above (https://github.com/bitshares/bitsharesjs/blob/master/lib/chain/src/ChainStore.js#L163), theoretically we can set the parameter to false, after got response, call get_objects(["2.1.0"]) again to subscribe correctly.

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

Actually the notify_remove_create parameter only affects notification of newly created objects and ID of removed objects, so 2.1.0 should not be affected since it's not new nor would be removed. After the set_subscribe_callback call, we should have code somewhere subscribing to 2.1.0. Please check if this change will break other things.

@svk31

This comment has been minimized.

Contributor

svk31 commented Mar 19, 2018

We can't really turn it on or off, set_subscribe_callback is a global call and is only called once, on app initialization. Without the subscribe callback the GUI receives no notification for any objects. Without notify_remove_create, the GUI receives no notification about deleted orders, new orders, new margin positions etc.

get_full_account has a per account subscription model that the GUI does make use of.

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

I'm not talking about stop calling the whole set_subscribe_callback, but only about the notify_remove_create parameter. So we analysis it case by case.

IMHO, deleted/filled orders, new orders and position changes should be notified if

  • subscribed to an account that is impacted by those operations, or
  • subscribed to the specified market (which is another call subscribe_to_market).

Once we've sorted out the logic, we can check if there are bugs, and fix them accordingly.

So, the approach is straight forward:

  • firstly, change the parameter to false in js, see what would go wrong;
  • if something went wrong, check and fix that part.
@svk31

This comment has been minimized.

Contributor

svk31 commented Mar 19, 2018

I agree with your points about how it should ideally work.

Setting it to false and seeing what breaks is not as easy as it sounds though, as the changes could be subtle, it can be hard to tell if something doesn't change that should have. As is the GUI depends on that boolean and will break without it.

Are you able to make a list of what types of objects are affected by the boolean?

By the way the api node spams a lot of stuff that was never asked for just from setting a subscribe call back. I've coded the chainstore to just ignore a lot of it, cutting down on that would certainly help api performance as well. An effort was made in the past to improve it but it's still pretty bad i think.

@wmbutler wmbutler changed the title from Performance improvement: calling set_subscribe_callback API with false to [10] Performance improvement: calling set_subscribe_callback API with false Mar 19, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

@svk31 as the in-code comment quoted in OP, if the boolean is set to true, API server will push every newly created object to the client, no matter whether the client explicitly subscribed, thus the "spamming" as you described. This ticket is for fixing this. The correct way is to set it to false. By setting it to false, API server will only push subscribed data to client; e.g. if you subscribe to an account, changes related to the account will be pushed, including transfers, orders and etc; if you subscribe to a market, changes in that market will be pushed, including new orders, fills, call order updates and etc. This is actually what you want.

@wmbutler

This comment has been minimized.

Contributor

wmbutler commented Mar 19, 2018

@abitmore do we need to tell nodes to set the boolean to false in combination with our UI work?

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

@wmbutler no, we won't ask API servers to do anything. Before UI get fixed, back end should always allow the parameter to be set to true.

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 19, 2018

@svk31 regarding filtering on client side, since the backend uses bloom filter which uses less RAM but naturally has false positives, the server will push a few unsubscribed data to client, so the client still need to do filtering. But that data should be limited to certain data types if set_subscribe_callback is called with false.

abitmore added a commit to bitshares/bitshares-core that referenced this issue Mar 19, 2018

Temporarily remove enable-subscribe-to-all option
TODO: revert this commit when this GUI issue is fixed: bitshares/bitshares-ui#1338

abitmore added a commit to bitshares/bitshares-core that referenced this issue Mar 19, 2018

Temporarily remove enable-subscribe-to-all option
TODO: revert this commit when this GUI issue is done: bitshares/bitshares-ui#1338
@abitmore

This comment has been minimized.

Member

abitmore commented Mar 20, 2018

@wmbutler : please feel free to assign this issue to other devs if @svk31 is too busy. I assigned it to him just because I thought he knows the most.

@wmbutler wmbutler modified the milestones: 180501, 180415 Mar 20, 2018

@wmbutler

This comment has been minimized.

Contributor

wmbutler commented Mar 20, 2018

@abitmore thx. I removed @svk31. He can of course choose to claim it. Thanks for your help here. Really pushing issues that lead to performance increases.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Mar 20, 2018

as @svk31 said there were efforts in the past to improve the subscription system but it is still not working properly. i requested test cases in the node side to fix this problem or at least being able to fully understand the limitations.

@svk31

This comment has been minimized.

Contributor

svk31 commented Mar 22, 2018

@svk31

This comment has been minimized.

Contributor

svk31 commented Mar 22, 2018

Setting the boolean to false doesn't appear to make much of a difference, I still get spammed with 2.8.x and 5.1.x and 5.2.x objects for example that I have no interest in.

@abitmore

This comment has been minimized.

Member

abitmore commented Mar 23, 2018

The data pushing might be triggered by something. By default if the boolean is set to false, no data will be pushed.

  • 2.8.x is impl_block_summary_object_type, which can be used to fill TaPos fields when signing transactions, but I'm not sure if UI is subscribing to it
  • 2.9.x is account history object
  • 5.1.x is bucket_object_type in market history plugin, aka data for candle chart
  • 5.2.x is market_ticker_object_type, aka latest price info

If some data are not expected please submit an issue to bitshares-core, then we can analyse and fix.

@svk31

This comment has been minimized.

Contributor

svk31 commented Mar 24, 2018

OK, we do use the 2.9.x objects but none of the others are used by the GUI.

I've been doing quote a bit of testing with the boolean set to false and it works much better than I expected. Both new/changed margin positions show up correctly, and placing and cancelling orders works fine as well. I'm OK with us setting it to false in staging in the GUI for now, which will allow developers and testers to discover potential issues. Unless we discover something major we can then include it in the 0415 release.

svk31 added a commit that referenced this issue Mar 24, 2018

svk31 referenced this issue Mar 24, 2018

@svk31 svk31 self-assigned this Mar 24, 2018

@abitmore abitmore referenced this issue Mar 24, 2018

Open

Review subscription API #777

0 of 7 tasks complete
@svk31

This comment has been minimized.

Contributor

svk31 commented Apr 1, 2018

Since I haven't seen any adverse effects I decided to set the boolean to false already in this 0401 release. If we discover something major we can always hotfix it.

@svk31 svk31 closed this Apr 1, 2018

@svk31 svk31 modified the milestones: 180415, 180401 Apr 1, 2018

nikitaKunevich pushed a commit to LocalCoinIS/LocalCoin-core that referenced this issue Jun 7, 2018

Temporarily remove enable-subscribe-to-all option
TODO: revert this commit when this GUI issue is done: bitshares/bitshares-ui#1338

jmjatlanta added a commit to bitshares/bitshares-core that referenced this issue Jun 13, 2018

abitmore added a commit to bitshares/bitshares-core that referenced this issue Sep 3, 2018

Cryptolero pushed a commit to CryptoBridge/cryptobridge-ui that referenced this issue Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment