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

Precalculate market ticker data #513

Merged
merged 2 commits into from
Dec 11, 2017

Conversation

abitmore
Copy link
Member

PR for #509.

Benchmark:

  • before change
$ time for n in {1..1000};do curl -d '{"id":1,"method":"call","params":["database","get_24_volume",["1.3.0","1.3.113"]]}' http://127.0.0.1:38380/ws >/dev/null 2>&1;done;echo

real    0m9.517s
user    0m3.020s
sys     0m1.408s
  • after change:
$ time for n in {1..1000};do curl -d '{"id":1,"method":"call","params":["database","get_24_volume",["1.3.0","1.3.113"]]}' http://127.0.0.1:38480/ws >/dev/null 2>&1;done;echo

real    0m5.185s
user    0m2.852s
sys     0m1.404s

API query speed slightly increased.

By the way, it seems the overhead is quite high, even an empty query takes around 5 seconds to complete (1000 times). I'm not sure why.

$ time for n in {1..1000}; do curl -d '{}' http://127.0.0.1:38380/ws >/dev/null 2>&1;done
real    0m4.777s
user    0m2.968s
sys     0m1.300s

@abitmore abitmore added this to the Next Non-Consensus-Changing Release - 201712 milestone Nov 30, 2017
@oxarbitrage oxarbitrage self-requested a review December 1, 2017 09:57
Copy link
Member

@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.

works really good and i like very much how it was implemented. the new index can be extended easy to make #512 and possible other similar that could be needed in the future.
in addition there is a nice increase in the speed of the calls, the new order book parameter does not affect current implementations as it is optional, the design is a lot better, etc.

not sure if @pmconrad haves time to take a look and check if we have anything missing but it looks all positive from here.

@pmconrad
Copy link
Contributor

pmconrad commented Dec 4, 2017

I've glanced over the changes, but it's a lot more than I expected...
I think you should add unit tests, it's close to impossible to test this manually.

Wrt the benchmark - connection setup is always quite expensive. For comparison, 1000x curl against a locally running apache just took 12 seconds on one of my servers. A test using a websocket connection should provide more meaningful results.

@oxarbitrage
Copy link
Member

The websocket test will be a good one if we assume for example the same application is getting the tickers for all the crosses. Will probably do this using the same connection websocket. The curl one is also a good one for cases where we are having multiple clients calling at the same time.

Scripting a websocket can be done with https://github.com/bitshares/bitshares-core/wiki/Scripting-websockets-easy

Ill try to get a small script to measure the time in the same connection.

@oxarbitrage
Copy link
Member

made a test with websockets by calling 1000 times.

node with changes:

alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull513/script $ python3 test.py 
0.21059155464172363
alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull513/script $ python3 test.py 
0.20846176147460938
alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull513/script $ 

here is against a node without the changes:

root@alfredo:~# python3 test.py 
1.1013147830963135
root@alfredo:~# python3 test.py 
1.1829383373260498
root@alfredo:~# 

for reference i am attaching the script:

import time
from websocket import create_connection
ws = create_connection("ws://localhost:8090")

start_time = time.time()

for x in range(0, 1000):

        ws.send('{"method": "call", "params": ["database", "get_24_volume", ["1.3.0","1.3.113"]], "id": 4}')
        result =  ws.recv()
        #print result

elapsed_time = time.time() - start_time
print(elapsed_time)

ws.close()

@oxarbitrage
Copy link
Member

We have no unit test for any market related stuff. We do have for account_history but nothing for market or trading. I think we can take this opportunity and create unit tests for the market api calls. It will need some work as we need to fill buckets and such from the market_tests.cpp file but in the long run i think it will worth it. We can get started with get_ticket, merge and then go adding more as we can.

@abitmore let me know if you want me to get started with that, if you think it will worth or if you prefer to make the first market test.

another thing i noticed is that the cli_wallet don't expose almost anything from the markets api except the order book. I think it will be nice to have get_ticker and get_volume at least. Maybe some efforts on adding some new trading calls to the cli will worth but unsure if too many people will care. this last is off topic and can be discussed in another issue.

@abitmore
Copy link
Member Author

abitmore commented Dec 6, 2017

  1. need to fix New market API uses doubles #144, otherwise some unit tests will likely fail.
  2. if no major issue is found, I want this PR to be included in next non-consensus release, which is best to be released in next week.
    So, my opinion is, unit tests are of course helpful, but not essential for this PR. If we don't have much time right now, we can postpone the work to next milestone. I personally won't work on unit tests in the near future, so @oxarbitrage if you think it is high priority, you can start working on it.

@oxarbitrage
Copy link
Member

thanks @abitmore . i agree with you in that it is not high priority but i think it will be good to have, i'll take care of getting started with some market tests after we can get over #144(added question there) but i also agree it can be merged without unit tests.

@abitmore abitmore merged commit ee06ec1 into bitshares:develop Dec 11, 2017
@abitmore
Copy link
Member Author

Merging this one. Created a new ticket (#523) for the unit testing thing.

@abitmore abitmore deleted the 509-prepared-ticker branch June 28, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants