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

Terrain render loop: Prioritize large batches and lazy framebuffer switches #10701

Merged
merged 10 commits into from
Jun 1, 2021

Conversation

karimnaaji
Copy link
Contributor

@karimnaaji karimnaaji commented May 21, 2021

Ports two different optimizations to the render cache from native.

  • (1) In terrain render cache prioritize large render batches first, this prevents allocating pool entry to small layer groups, which eventually leads to more draw call when the render cache pool is fully used. In an example location this changed FPS from 44 to 55.
  • (2) In the terrain render loop, prevent overly binding/clearing framebuffers when nothing was drawn in a render batch. Unnecessary binding/clears are expensive gl calls. The full stats are attached in the before/after below. In an example frame this reduced the number of framebuffer bindings from 228 to 88 and number of clears from 103 to 71. This prevents such sequence of unnecessary bindFramebuffer:

Screen Shot 2021-05-19 at 8 23 04 PM

before/after (1)

Screen Shot 2021-05-19 at 6 04 52 PM Screen Shot 2021-05-19 at 6 04 03 PM

before/after (2)

Screen Shot 2021-05-21 at 2 06 24 PM Screen Shot 2021-05-21 at 2 06 16 PM
  • <changelog>Improve terrain performance by reducing number of framebuffer switches during draping</changelog>
  • <changelog>Improve terrain performance by prioritizing allocation of large render cache batches</changelog>

@karimnaaji karimnaaji added 3d 📐 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage labels May 21, 2021
@astojilj
Copy link
Contributor

There seems to be an issue with symbols in optimizeForTerrain: false mode.

@karimnaaji karimnaaji marked this pull request as draft May 25, 2021 19:18
@karimnaaji karimnaaji force-pushed the karim/prioritize-large-batches branch from 15c1385 to 3c604bb Compare May 27, 2021 21:25
@karimnaaji
Copy link
Contributor Author

Thanks for the catch, it was missing a reset to main framebuffer (fixed by 995f819 + assert in 3c604bb)

@karimnaaji karimnaaji marked this pull request as ready for review May 27, 2021 21:29
@karimnaaji karimnaaji force-pushed the karim/prioritize-large-batches branch from 3c604bb to 8c78724 Compare May 27, 2021 22:01
@karimnaaji karimnaaji force-pushed the karim/prioritize-large-batches branch from 8c78724 to 00f0643 Compare May 28, 2021 00:36
Copy link
Contributor

@astojilj astojilj left a comment

Choose a reason for hiding this comment

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

👍

@karimnaaji karimnaaji merged commit 572bf33 into main Jun 1, 2021
@karimnaaji karimnaaji deleted the karim/prioritize-large-batches branch June 1, 2021 15:51
Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

Sorry I was slow on the review! I only had the tiniest nitpick and put most of my energy into working to understand the PR. I have a couple questions I'll follow up with after the fact.

drawTerrainRaster(painter, this, this.proxySourceCache, accumulatedDrapes, this._updateTimestamp);
this.renderingToTexture = true;

accumulatedDrapes.splice(0, accumulatedDrapes.length);
Copy link
Contributor

@rreusser rreusser May 28, 2021

Choose a reason for hiding this comment

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

Super nitpicky: I had to double-check what this does in the inspector. Maybe setting length is clearer if the intention is to simply flush the array?

Suggested change
accumulatedDrapes.splice(0, accumulatedDrapes.length);
accumulatedDrapes.length = 0 ;

SnailBones pushed a commit to SnailBones/mapbox-gl-js that referenced this pull request Jul 15, 2021
…itches (mapbox#10701)

* Prioritize assignment of large render batches first

* Prioritize pool assignment of large render batches first

* Simplify unnecessary branch

* Simplify render loop

* Fix lint

* Lazy framebuffer switches

* Use non-indexed for each loop

* Fix missing reset of backbuffer when not rendering any drapes

* Ensure main framebuffer reset after terrain render

* Simplify
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3d 📐 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants