-
Notifications
You must be signed in to change notification settings - Fork 135
Feature: Throttle listener notifications #195
Conversation
This PR was deleted due to changing the default branch name from |
Arg.. 😉 didnt notice the name change.
If necessary we can prepare that. But it didt face the same prob on ios ...
yet
Thomas Vidas ***@***.***> schrieb am Do., 18. Nov. 2021,
05:04:
… This PR was deleted due to changing the default branch name from master
to main. Not because it wouldn't be a good feature 😄. I would not be
opposed to adding this feature behind a request flag, but it would need to
have parity on iOS as well and not just Android
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#195 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABSTO2NTPWAYUNVCN6CM23UMR3MLANCNFSM5HQ3ENYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Also probably parity with web since its unthrottled too if we go for the perfect score I have nothing against throttling by using intervals (its simple and works) but as an alternative to adding new feature flag I would propose running progress emit asynchronically and chaining promises/async tasks while cancelling all the tasks after the current running one (to avoid backlog) / calling emit only when emit task ended calling so that way we ensure only the latest one is being emitted and shouldn't block as much as synchronic notifyListeners. I haven't really tested that since it worked okay for me (I did the initial progress implementation) without throttling but I would try to go this way so we can avoid additional flag if it's possible (assuming it's more efficient/slightly less than interval but still much better than unthrottled). |
@jkbz64 sounds like a fantastic solution, i am not a java programmer so this is out of my scope :/ but i'll be happy to help wher i can |
@phiamo |
@jkbz64 thanks a lot for this beatyful implementation! My feeling is its emiting soo many messages and somehow android webview and capacitor is not abled to handle that:
i also think this might be connected with debug logging of capacitor, since i can reproduce this dump even with all of my code commented out, just by connecting the chrome inspect js debugger to the webview. and per emit in android one addListener debug statement is printed to the console until ... it crashs if i again add a delay like in the following snipplet, everything works like a charm:
i am not sure where you see the responsibility for the underlying problem, imho it would be nice if the http plugin would throttle this so that the developer experience is not tainted, but maybe documenting this maybe even edge case would suffice ? for sure i found out, if i use a ngzone.run inside that listener without throttling the webview crashes even faster. |
@phiamo Just want to point out, my idea is not set in stone and I'm not in the power to decide, just wanted to see if this method would fix your performance issues and we could iterate upon that together.
I haven't really done much research about this, just knew from the start the frequency of the requests might be too much for some cases. I assumed the problem was just that notifyListeners is blocking the download loop for too long. In my implementation I was trying only to address slowdown of the download, not to solve the "problem" of too many emits.
In some cases "too many emits" might be helpful in other not so much. For a file that downloads in less than 300ms by using your method we will get emit only once so because of that (at least I think) @thomasvidas doesn't want to make it non-configurable (and kinda hassle, for every file smaller you might want to configure delay separately). If we want to use the emit delay we are back to having to introduce new options parameter which I wanted to avoid entirely, and if that doesn't work out we might as well just drop the async idea and do it your way
If we really want to add some kind of "delay" to the async way I would just plug in some really small non-configurable value to the Thread.sleep (like 30/50) in ProgressTask instead of Thread.yield, that reduced my emits from 250calls to 15-20calls and shouldn't affect most downloads but that is still opinionated (the 30/50 delay) which I don't like. Also I just noticed, your initial (the one from PR) implementation would not [always] emit the completion progress event, which might be helpful for someone (e.g 250kb/250kb downloaded). |
Hi,
Thanks for the amazing plugin!
However i noticed while downloading larger files (~50 to 100mb) and tring to use the progress listener, that so many capacitor notifyListeners calls are created, that this slowed down the whole app as well as the download itself.
when throttling this to 1 per second everything wokrs very well.
maybe it would be a good idea to have the throttle interval configurable and disableable?
Cheers Phil