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

Bugfix/improvement: Orderbook/ticker update processing #364

Merged

Conversation

@gloriousCode
Copy link
Collaborator

commented Oct 4, 2019

image

Captured footage of GoCryptoTrader attempting to outrun Coinbase Pro updates

Description

There have been many updates to websockets, orderbooks and tickers recently that rely on the use of maps. Every time a map is used, go has to look it up - you'll see in the old code, we were looking up items many lines in a row. At the end of our maps are pointers, so we can just make a var referencing the pointer without needing to do a double-lookup.

Coinbase Pro sends out websocket orderbook and ticker updates for every change. When processing this large amount of data, GoCryptoTrader would fall behind leading to an ever larger queue of updates. This was captured by @shazbert as a result of testing #360 (noscope)

This PR addresses that by minimising map lookups, greatly increasing efficiency. Check out the following benchmarks:
Before

BenchmarkUpdateBidsByPrice-8              	     242	   4810067 ns/op
BenchmarkUpdateAsksByPrice-8              	     254	   4901673 ns/op
BenchmarkBufferPerformance-8              	  131808	     10007 ns/op
BenchmarkBufferSortingPerformance-8       	  117646	     10404 ns/op
BenchmarkBufferSortingByIDPerformance-8   	  115381	     10201 ns/op
BenchmarkNoBufferPerformance-8            	  118810	      9999 ns/op

After

BenchmarkUpdateBidsByPrice-8              	   10000	    115699 ns/op
BenchmarkUpdateAsksByPrice-8              	   10000	    115897 ns/op
BenchmarkBufferPerformance-8              	 1000000	      1245 ns/op
BenchmarkBufferSortingPerformance-8       	  920434	      1298 ns/op
BenchmarkBufferSortingByIDPerformance-8   	  920463	      1310 ns/op
BenchmarkNoBufferPerformance-8            	 1243474	       947 ns/op

Now, even when enabling all coinbasepro currencies, disconnecting shows that there is no longer a queue of updates to be processed.

This PR also Fixes the following data race
https://travis-ci.com/thrasher-corp/gocryptotrader/jobs/241323694#L340

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

  • Bug fix (non-breaking change which fixes an issue)
  • Improvements to existing code

How Has This Been Tested?

I wrote the above benchmarks, and go test ./... -race and also tested by disconnecting the interenet connection and monitoring whether GCT lagged behind in orderbook updates.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules
gloriousCode added 2 commits Oct 4, 2019
…nimising multiple map lookups. Changes buffer to use pointers. Ensures orderbook benchmarks are on equal footing and set correctly. Removes data race by not setting waitgroup adds inside go routine causing wait and add to happen simultaneously. Updates ticker and orderbook service to use a pointer var instead of map lookups when setting data.
@gloriousCode gloriousCode self-assigned this Oct 4, 2019
@codecov

This comment has been minimized.

Copy link

commented Oct 4, 2019

Codecov Report

Merging #364 into engine will increase coverage by 0.01%.
The diff coverage is 96.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           engine     #364      +/-   ##
==========================================
+ Coverage   42.54%   42.56%   +0.01%     
==========================================
  Files         152      152              
  Lines       34018    34020       +2     
==========================================
+ Hits        14474    14479       +5     
+ Misses      18583    18580       -3     
  Partials      961      961
Impacted Files Coverage Δ
exchanges/websocket/wshandler/wshandler.go 73.46% <0%> (+0.18%) ⬆️
exchanges/ticker/ticker.go 98.43% <100%> (+0.01%) ⬆️
exchanges/orderbook/orderbook.go 98.66% <100%> (ø) ⬆️
exchanges/websocket/wsorderbook/wsorderbook.go 93.03% <96.87%> (ø) ⬆️
dispatch/dispatch.go 88.42% <0%> (+0.52%) ⬆️
common/common.go 88.69% <0%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ca1e5c...90d8a97. Read the comment docs.

…id orderbook updates via websocket. Updates benchmarks to be consistent
Copy link
Collaborator

left a comment

tACK - Awesome work, this is definitely a massive improvement!

@thrasher-

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2019

tACK, great performance gains!

@xtda
xtda approved these changes Oct 4, 2019
Copy link
Member

left a comment

Tested approved :D

@thrasher- thrasher- merged commit 62a528a into thrasher-corp:engine Oct 4, 2019
4 of 5 checks passed
4 of 5 checks passed
Review Action 0 issues found before hitting an error.
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 96.47% of diff hit (target 42.54%)
Details
codecov/project 42.56% (+0.01%) compared to 8ca1e5c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.