capacitor-community / http

Community plugin for native HTTP
MIT License
208 stars 136 forks source link

Http progress listener has serious performance issue on Android #267

Open fireonmac opened 2 years ago

fireonmac commented 2 years ago

Is your feature request related to a problem? Please describe. Http.AddListener('progress', (e) => ...) has serious performance issue when downloading large file(about 50mb) on Android device. I used it to show the progress of file download, but it makes my app too slow.

Describe the solution you'd like It would be great to behave just like it does on IOS

Describe alternatives you've considered How about limit frequency of emitting progress event only on Android

Additional context This problem already seemed to be discussed in #195, but it was closed and still remains unfixed.

jkbz64 commented 2 years ago

Note: I'm not a maintainer but I did write the initial progress implementation.

iOS implementation uses .default NSURLSession which under the hood most likely uses variable "buffer" size depending on your device/network connection, whenever URLSessionDownloadDelegate calls back the urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didWriteData bytesWritten: Int64, totalBytesWritten: Int64, totalBytesExpectedToWrite: Int64) it returns bytesWritten incremented by the "buffer" length (assuming the buffer gets filled every time) and also might potentially postpone the call to the method so it may feel "better".

Currently the android implementation uses fixed buffer of 1024 bytes which will make roughly 100k calls for 100MB file to the progress listener - again, assuming the buffer gets filled everytime

@thomasvidas

Is there any specific reason buffer size of 1024 bytes was chosen? I think typical size for the network packet will be power of two in the range of 512b -> 32kb. Raising the buffer size to let's say 8192 bytes would reduce number of listener calls by almost ten times on fast connections (where the problem is very apparent) and probably would alleviate the problem without having to go into much more complicated solutions for reasonable sized files. If you have nothing against I can open the PR, doesn't seem like a big deal considering progress isn't usually used for very small files.

jkbz64 commented 2 years ago

To dampen the issue I would advise you @fireonmac to make sure you are removing the listeners after you have created them and don't need anymore and/or early exit for the specific files - or simply just do not subscribe multiple times, it will double the number of callbacks.

If you happen to use react or other library/framework which re-render children on component state change (where you might store the number of downloaded bytes), make sure the stored progress change doesn't actually do that!

nurfgun commented 2 years ago

am on the same team with @fireonmac. and i can confirm that we make sure that only one listener is attached at all time and the component containing percentage get refreshed only once in a while with a throttle. Not so much difference it makes thou. We are currently using a fork of this repo with #195 as a temporary workaround and hope this resolves soon! Thanks for your great work, @jkbz64 btw.