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

Refactor scheduling events in render thread #1068

Merged
merged 19 commits into from
Feb 3, 2022

Conversation

kiryldz
Copy link
Contributor

@kiryldz kiryldz commented Jan 20, 2022

Summary of changes

MapView#queueEvent will be executed properly on Mapbox render thread. Previously scheduling such events e.g. when render was not created properly could lead to a crash / be skipped if running OpenGL ES commands there.

Additionally internal render scheduling logic was refactored / fixed:

  • in case of any EGL setup issues rescheduling will happen in small delay and not immediately in order not to overload render thread message queue
  • user events scheduled by MapView#queueEvent(runnable, needRender = true | false) will be executed on render thread even if it's not fully prepared. In this case such events will keep to get re-scheduled with small delay (50 ms) until render thread is either ready or killed.
  • any user render events scheduled by MapView#queueEvent(runnable, needRender = true) will be executed after map draw calls but before swapping buffers
  • non-render events (both scheduled by MapClient#scheduleTask and MapView#queueEvent(runnable, needRender = false)) now have a check to verify render thread is fully ready.
  • extra render call will be always scheduled now when app is brought back to front.

User impact (optional)

Some data collected by gfxinfo by running benchmarks changing map camera.

Average values for easeTo benchmark (5 runs, infinite moving camera between 3 points for 45 seconds).

metric main branch current refactor diff
jankyFrames 3.0 2.75 -8.3333%
numberSlowUiThread 2.5 2.0 -20.0%
numberFrameDeadlineMissed 2.5 2.25 -10.0%
jankyFramesPercent 0.21 0.193 -8.0952%
totalFramesRendered 1406.5 1422.5 +1.1376%

Average values for flyTo benchmark (5 runs, infinite moving camera between 3 points for 45 seconds).

metric main branch current refactor diff
jankyFrames 3.25 3.25 0%
numberSlowUiThread 2.75 2.5 -9.0909%
numberFrameDeadlineMissed 2.75 2.75 0%
jankyFramesPercent 0.245 0.242 -1.2245%
totalFramesRendered 1332.5 1335.75 +0.2439%

All the metrics look slightly better with refactor introduced in this PR although it's hard to gather very stable ones. Main idea is that code looks cleaner and less redundant actions are performed in rendering logic.

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Fix dropping / crashing in user events scheduled on a render thread with MapView#queueEvent. Refactor scheduling logic for render thread in general slightly improving rendering performance.</changelog>.
  • If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: #1044

PRs must be submitted under the terms of our Contributor License Agreement CLA.

@kiryldz kiryldz added the bug 🪲 Something isn't working label Jan 20, 2022
@kiryldz kiryldz requested a review from a team January 20, 2022 09:09
@kiryldz kiryldz self-assigned this Jan 20, 2022
@kiryldz kiryldz marked this pull request as draft January 20, 2022 16:23
@kiryldz kiryldz requested a review from yunikkk January 21, 2022 15:28
@kiryldz kiryldz marked this pull request as ready for review January 25, 2022 09:17
Copy link
Contributor

@Chaoba Chaoba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kiryldz kiryldz force-pushed the kdz-renderer-check-egl-queue-events branch from 180673a to 2364753 Compare February 1, 2022 12:03
@kiryldz kiryldz requested a review from yunikkk February 1, 2022 12:04
@kiryldz kiryldz force-pushed the kdz-renderer-check-egl-queue-events branch from 2364753 to 13d4fa3 Compare February 1, 2022 12:07
@yunikkk yunikkk self-requested a review February 1, 2022 13:48
CHANGELOG.md Outdated Show resolved Hide resolved
@kiryldz kiryldz force-pushed the kdz-renderer-check-egl-queue-events branch from 51be911 to 727141e Compare February 2, 2022 16:40
@kiryldz kiryldz merged commit f90b7b0 into main Feb 3, 2022
@kiryldz kiryldz deleted the kdz-renderer-check-egl-queue-events branch February 3, 2022 08:38
@kiryldz kiryldz mentioned this pull request Apr 27, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with MapView#queueEvent method
6 participants