search

shadowk29 / BinanceBalance

Simple graphical interface to facilitate balancing of user-defined cryptocurrency portfolio on Binance. This is in no way affiliated with Binance.
MIT License
8 stars 5 forks source link

Added multiprocessing and threading #4

Closed thes3cr3t1 closed 5 years ago

thes3cr3t1 commented 5 years ago

Added multiprocessing to add messages to the message queue from a separate process in order to save the GUI from blocking.

Added threading to process messages from the queue and update the GUI faster.

Still a lot of work to do here, but definitely moving forward IMO.

Would like to enable trading with BTC as the base currency so that trades for BTCUSDT, BTCUSDC, etc can be made.

shadowk29 commented 5 years ago

There's a lot to dig through here, impressive first contribution. I will have to take some time to go through it carefully before I merge, but some initial impressions: It is generally preferable to avoid global variables wherever possible. If you need to pass variables between different classes, a good way to do it might be to make a request_data() function for the class that needs to provide data, which then it called by the other class whenever it needs a particular value. I will dig into this some more when I have time to sit down and test properly. Not the end of the world, if you prefer I can clean that part up once I am able to learn how the threading works, since I'm new to that as well. Thanks very much for your time!

thes3cr3t1 commented 5 years ago

Yes, completely agree regarding the global variables. as you have noticed I was struggling with passing the variables between the classes. I'll look toward implementing your solution.

In the mean time, feel free to edit my pull requests as you see fit. Its likely some changes will be required in places.

I'll try to keep the code cleaner on future requests, but here was a lot of restructuring here.

In my testing, the threads definitely help to increase the speed and reduce the message queue. Inputting data into the message queue was also using a lot of resources and causing the GUI to block. Hopefully, now you will notice that the GUI blocks less often. There is still plenty of room to optimize the functions so that threads have to work less.

I have tested using different amount of threads, there doesn't seem to be a one size fits all approach to increasing the efficiency, in terms of the amount of threads used. Its my belief that the maximum amount of threads used should be dynamic. I have therefore tried to implement a mechanism that increases the thread count when the message queue is increasing, and decreases the thread count when the queue starts to reduce. This should also increase performance when the code is running on different machines with different CPU speed/Cores.

I also changed the web sockets to use the multiplex socket from Binance, hopefully there are some performance gains there.

shadowk29 commented 5 years ago

I will spend some time reading your changes on saturday and merge the pull request then.

Re: thread count, I have not yet gone through in detail how you handled it, but be careful of having multiple threads writing to the same data structure, as this can cause issues depending on how the queue class handles threaded IO. Sounds like you tested carefully so I am not too worried, but something to keep in mind when counting threads. If the performance can be realized in this way, the ideal structure in my head would be one thread receiving messages and adding them to the queue, one thread reading and processing messages from the queue and updating the backend information, and one thread updating the GUI from the backend. That way only one thread writes to any given data structure, and only one thread reads from any given data structure. But for very large portfolios maybe this is not enough?

Out of curiosity, how many coins are in the portfolio that gives you issues? I have 15 coins running on this without issue, but it could be a difference in CPU speed.

thes3cr3t1 commented 5 years ago

My understanding is that the threads don't run concurrently, especially threads running in the same process. So with that you have to pause the execution at some point so that other threads can perform their tasks. With that in mind, I think the best way is to use a separate process for inputting data into the message queue. With a large portfolio the data is coming in that fast that its difficult to process.

With regard to reading data from the message queue and updating the GUI, it may well be a better approach to do it as you described. Again, execution may have to pause to allow the GUI thread to do its bit. The best thing about this approach is that if we determine that threads for reading the message queue or updating the GUI need to be increased, it would be trivial to do so. It would definitely be interesting to test the GUI being updated by a separate worker thread.

I also noted that tkinter is not threadsafe.

I was running into difficulty running the main branch of code at around 8-9 coins, since then I have been able to run it with 15 coins. There is still a bit of error checking to do with the threads, every so often I get a run time error. I don't think I am handling the closing of threads as well as it could be.

I also identified a couple of lines of code which were at least for me, causing a huge bottleneck. these lines were 576,577 in the update_actions() function.