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.

Sign up for GitHub

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

Jump to bottom

Unnecessary map initialization performance penalty caused by delaying composing GoogleMap() subcomposition until after GoogleMap object is available #501

Open
bubenheimer opened this issue Jan 5, 2024 · 7 comments
Open

Unnecessary map initialization performance penalty caused by delaying composing GoogleMap() subcomposition until after GoogleMap object is available #501

bubenheimer opened this issue Jan 5, 2024 · 7 comments
Labels
triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@bubenheimer
Copy link
Contributor

bubenheimer commented Jan 5, 2024 •
edited

android-maps-compose 4.3.0

From code inspection I've noticed that there is an undue performance penalty caused by delaying the GoogleMap() subcomposition until after the GoogleMap object has been made available via suspending MapView.awaitMap().

While the GoogleMap object is required during the "apply changes" phase of the subcomposition, it should not be required to perform the initial composition of the subcomposition, prior to "apply changes".

Initial composition phase of subcomposition and MapView.awaitMap() should and can be performed in parallel.
@bubenheimer bubenheimer added triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jan 5, 2024
@bubenheimer bubenheimer mentioned this issue Jan 5, 2024
Open
@bubenheimer
Copy link
Contributor Author

Just as an aside thought, not directly linked to this issue: the initial composition phase of the GoogleMap() subcomposition theoretically would not need to be performed on the main thread, which would free the main thread for other concurrent initialization tasks. This may be too risky to attempt for now.

bubenheimer added a commit to bubenheimer/android-maps-compose that referenced this issue Jan 5, 2024
@bubenheimer
Delay GoogleMap object access until composition apply phase (see goog…
9215d1b 
…lemaps#501)
@bubenheimer
Copy link
Contributor Author

Another element to consider here: MapsInitializer may need to be a part of an app's map initialization pipeline: the GoogleMap() subcomposition composition phase should not be delayed until after MapsInitializer is done; it can run in parallel.

bubenheimer added a commit to bubenheimer/android-maps-compose that referenced this issue Jan 10, 2024
@bubenheimer
Delay GoogleMap object access until composition apply phase (see goog…
001a8ea 
…lemaps#501)
dkhawk pushed a commit that referenced this issue Jan 31, 2024
@bubenheimer
fix: Use recomposed map listener callbacks rather than only the initi…
d14daba 
…ally … (#478)

* fix: Use recomposed map listener callbacks rather than only the initially composed version
Fixes #466

* Minor formatting fix

* Label leaf composable as @GoogleMapComposable for proper compile-time diagnostics

* Clarify documentation

* Address spurious subcomposition recompositions by delaying state updates to after parent composition, not during parent composition

* Delay GoogleMap object access until composition apply phase (see #501)

* Revert "Address spurious subcomposition recompositions by delaying state updates to after parent composition, not during parent composition"

This reverts commit dff2b0a.
googlemaps-bot pushed a commit that referenced this issue Jan 31, 2024
@semantic-release-bot
chore(release): 4.3.3 [skip ci]
f857fc4 
## [4.3.3](v4.3.2...v4.3.3) (2024-01-31)

### Bug Fixes

* Use recomposed map listener callbacks rather than only the initially … ([#478](#478)) ([d14daba](d14daba)), closes [#466](#466) [#501](#501)
@bubenheimer
Copy link
Contributor Author

@wangela Just FYI: the 4.3.3 release says that it fixes this issue, but that is not the case. I think this mixup happened because I mentioned this issue in one of the commits for 4.3.3 ("see #501", I was not aware that this has "fixes" semantics, maybe some problem with release automation, or maybe I shouldn't do this in commit messages?).

@wangela
Copy link
Member

wangela commented Feb 27, 2024

@bubenheimer Where are you seeing that the release says it fixes this issue 501? I only see it mentioning that it fixes issue 466 which was tagged intentionally on your PR number 478.

@bubenheimer
Copy link
Contributor Author

@wangela That link says it "closes" #501

@wangela
Copy link
Member

wangela commented Feb 27, 2024

Strange, may be human error. I've deleted that mention from the release notes.

@bubenheimer
Copy link
Contributor Author

I've added two related issues on the Google issue tracker:

https://issuetracker.google.com/issues/340494390
https://issuetracker.google.com/issues/340476594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bubenheimer @wangela