-
Notifications
You must be signed in to change notification settings - Fork 536
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 test coverage #171
improve test coverage #171
Conversation
Travis says that |
This is my last change for the throttling, it seems... How could we force this exception in a test case? Just ignoring these lines (pragma: no cover) is not really a good solution... |
I added a test and now coverage is 100% in Travis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @AndreyNikiforov ! Makes sense to go for single threading here, even though I'm still a little unsure about the multithreaded downloads. Multithreading can be a beast and I remember we had issues with it last year. I did not find the calm yet to dive into the current implementation...
@menkej I am skeptical about multithreaded downloading from a single source with a goal to improve speed because 1) network speed will be a limiting factor if no throttling at the source 2) if throttled at the source, it may be too naïve to expect abuse prevention to be implemented at connection level only. I would be curious to see some test results (single threaded vs multi threaded downloading) |
I run some test downloads. I am on a 100 MBit connection. No clear lab conditions - Netflix and Skype session running. Downloading 1000 recent files. default setting (multithreaded) --threads-num 1 Just three runs. Just 1000 pics. Seeing not much of a difference really. Multithreading might make sense for parallel image/video conversions and stuff. For download... not so sure... Maybe it makes a difference on a clear line with a big pipe... |
Thanks for running tests, @menkej ! We can look at deprecating multithreading support eventually to reduce complexity |
I ran some tests on this tonight. My home internet connection is only 36Mbps. Here's two log files generated: Multi-threaded - https://pastebin.com/6RKYfgsy Results:
To sum up, I see a significant performance benefit to multi-threaded downloads. However, multi-threaded results in occasional errors e.g.:
|
@boredazfcuk thanks for measuring time! do you mind repeat same test multiple times to eliminate of any possible network speed fluctuations etc? I think it would be helpful to add a summary at the end of the the icloudpd run with total bytes downloaded, overall time, avg speed, and may be something else. Will be valuable beyond just perf tests. |
Sorry for not getting back sooner guys... I was busy last night and most of tonight too. I've run the tests again, but I've only had time to do one of each. The Mrs had a late night and she kills my connection with Netflix until she falls asleep lol I should have more time tomorrow to repeat again, hopefully two or three times. The results were pretty much the same as yesterday, multi-threaded was significantly faster, with single threaded taking nearly twice as long:
Log files: |
I managed to do some more testing overnight, as I put a speed test routine into my container and kicked off 8 runs (4-multi and 4-single). I am no longer seeing the large delays on single threaded downloads. Here are the details and times calculated: Multi: Single There seems to be a bug in my container that calculates the total time taken incorrectly, which is why they all show as taking 20:32 in the log files. When calculating manually, the times shown above are accurate. I'm not sure why this is different to before. As I could reliably reproduce it before under the same network conditions. These times have been taken using the latest version which defaults to single threaded. |
@boredazfcuk thanks a lot for the test! Not we can proceed with #180 with confidence. |
Time calculation bug should have been fixed and I've pushed latest version to DockerHub. People should be able to replicate my tests using this script I created:
|
No description provided.