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

Android PlatformBitmapBuilder calls expensive method in ctor #586

Closed
codedevote opened this issue Sep 18, 2020 · 6 comments · Fixed by #598
Closed

Android PlatformBitmapBuilder calls expensive method in ctor #586

codedevote opened this issue Sep 18, 2020 · 6 comments · Fixed by #598

Comments

@codedevote
Copy link

Hi,

I stumbled across a performance issue during initialization of PlatFormBitmapBuilder on Xamarin Android. It takes almost 5 secs in our current project to finish and we this isn't a big project, but rather around 30 (vector) drawables. They were converted from svgs and are not just simple icons, so some (3-5) of them have a pretty decent size (~300k).

I had a look in the code and I assume that calling GetDrawableList() from the ctor isn't optimal. In my opinion, such a "complete init" from all drawables should rather be a choice available to the user. Here is the link to the code I inspected:

I'd suggest, that instead of calling such potentially expensive code from a ctor, it should be refactored to a method that would also return a Task and perform that expensive calls concurrently (though I am currently not 100% sure, if the UI thread needs to be part of the solution - would be great if not). This may also have impact on cached information, so code may also need some adaptions with regard to the cache being not available and support on-demand-loading in that case.

In order to prevent this from being a breaking change, we could introduce a ctor parameter with a default value, so existing code won't break. Maybe you decide to mark the old ctor Obsolete and remove the old ctor completely with the next major version. This is of course your decision, I just wanted to draw a path to the future.

I think I could send a PR for this, just wanted to align on the topic before diving into.

@glennawatson
Copy link
Contributor

glennawatson commented Sep 18, 2020

Yeah happy to have a pr for a on demand load mode.

@codedevote
Copy link
Author

Glad to help out :-) Just to make sure, you give me your valuable insights on the UI thread part (would save me some time). I'd prefer kind of "async initialization pattern", but that won't succeed, if loading the drawables needs to happen on the UI thread. Thoug I am not sure if you (pre)load all the drawables or just read out all resource ids. In the latter case I'd assume offloading to a background task will not be a problem, while in the former case I'd be happy if you share your thoughts before I start.

@dpvreony
Copy link
Member

dpvreony commented Sep 22, 2020

relates to #421 - can you grab some figures from your immediate window to indicate if it's a similar thing of 100+ dll's getting loaded in debug. need to find a way to improve this methods overhead even if it moved to background :)

there is an android test runner that deploys to emulator that can be used to check it over ui threading issues. might need some test image assets adding to it

@dpvreony
Copy link
Member

dpvreony commented Sep 30, 2020

been tracking the perf, appears to be an issue with the repeat Debug output. having changed it to use string builder and only 2 calls to debug i've got it down from 1.6s in the test harness to ~100ms.

image

@codedevote
Copy link
Author

Thanks @dpvreony for fixing it already. I scheduled time this weekend to have a look at it, but you were faster 🥇

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants