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

Improve get_trade_history API performance #458

Closed
abitmore opened this issue Nov 3, 2017 · 9 comments
Closed

Improve get_trade_history API performance #458

abitmore opened this issue Nov 3, 2017 · 9 comments

Comments

@abitmore
Copy link
Member

abitmore commented Nov 3, 2017

The performance is terrible if start is too small.

@abitmore abitmore added the api label Nov 3, 2017
@oxarbitrage
Copy link
Member

yea, checked, really sucks. i think a binary search against the date like the one done here : https://github.com/bitshares/bitshares-core/pull/405/files?diff=unified can improve the call speed in all situations. let me know what do you think.

@oxarbitrage
Copy link
Member

lets see if we can pass this first: #455 before sending code to the same call.

@abitmore
Copy link
Member Author

Just noticed that bitshares-ui is not using this API. I'm not sure what application is using it. Since the pagination was largely broken (see #448), it's fair to guess that no application is relying on it. The get_ticker API was relying on it, but has been changed in #455.

I propose to change the behavior of this API to silently return an empty set if need to skip too many rows (E.G. 100) due to a too small start.

Thoughts?

@pmconrad
Copy link
Contributor

pmconrad commented Nov 10, 2017

Rename it to get_trade_history_deprecated and remove it after the next release. If anyone complains about the renaming we know they have been using it.

@abitmore
Copy link
Member Author

Still need to use it to load the first page, so I don't want to deprecate it completely. See #448 .

@pmconrad
Copy link
Contributor

Uh...

it's fair to guess that no application is relying on it.

Still need to use it to load the first page

Is is still in use or not?

@abitmore
Copy link
Member Author

When I was saying "fair to guess that no application is relying on it", "it" means the pagination.

Rather than deprecating it, I would add an index (base + quote + time) so we can efficiently query with a time range.

@pmconrad
Copy link
Contributor

Got it.

@abitmore abitmore self-assigned this Nov 13, 2017
abitmore added a commit to abitmore/bitshares-core that referenced this issue Nov 13, 2017
@abitmore
Copy link
Member Author

I think this can be closed as well. Fixed in #478.

@abitmore abitmore added this to the Next Non-Consensus-Changing Release - 201712 milestone Nov 27, 2017
@abitmore abitmore removed their assignment Mar 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants