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.

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

[GRPH-81] Different performance optimization #116

Merged
merged 17 commits into from
Nov 20, 2019
Merged

[GRPH-81] Different performance optimization #116

merged 17 commits into from
Nov 20, 2019

Conversation

gladcow
Copy link

@gladcow gladcow commented Sep 6, 2019

Backport of the Bitshares PR bitshares/bitshares-core#1180.

It adds several optimization:

  • cache several objects as class members instead of recreation every time we need them;
  • optimize update_expired_feeds with tracking of the expired rates;
  • a set of minor optimizations;

@bobinson bobinson added the Changes Requested Changes Requested via review label Sep 8, 2019
@gladcow
Copy link
Author

gladcow commented Sep 9, 2019

Also , this code doesn't change the functionality itself, so we can use existing chain_test and 'app_test` tests to check if it is correct. I hope it addresses the issues you have pointed on, so, re-review, please, @bobinson

@bobinson bobinson self-requested a review September 11, 2019 18:49
abitmore and others added 16 commits November 8, 2019 15:37
The old bug is cryptonomex/graphene#615 .

Due to the bug, `update_median_feeds()` and `check_call_orders()`
will be called when a feed is not actually expired, normally this
should not affect consensus since calling them should not change
any data in the state.

However, the logging indicates that `check_call_orders()` did
change some data under certain circumstances, specifically, when
multiple limit order matching issue (#453) occurred at same block.
* bitshares/bitshares-core#453
* changed an `assert()` to `FC_ASSERT()`
* replaced one `db.get(asset_id_type())` with `db.get_core_asset()`
* capture only required variables for lambda
@gladcow
Copy link
Author

gladcow commented Nov 8, 2019

I've updated the state, re-review, please.

@oxarbitrage
Copy link

From bitshares PR bitshares/bitshares-core@78ff14e is not included. Does is not apply for us ?

@gladcow
Copy link
Author

gladcow commented Nov 15, 2019

@oxarbitrage , I don't sure that skipping this check is acceptable for Peerplays network, because we haven't details about the workers now.) I've thought that we can add this commit later, it is not criticaltical. But I can add it if you think we should apply it right now.

@oxarbitrage
Copy link

I think we should add if we can so we will have the full PR from bitshares applied and will not have to get back later. Thanks.

@gladcow
Copy link
Author

gladcow commented Nov 20, 2019

@oxarbitrage , I've added this commit, re-review, please.

Copy link

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

thanks.

@oxarbitrage oxarbitrage merged commit e7e0816 into develop Nov 20, 2019
@bobinson bobinson added this to the v0.2 milestone Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants