Skip to content

Optimize matrix inversions and reduce GPU stalls #7367

Open
xavierjs wants to merge 15 commits intomaplibre:mainfrom
xavierjs:xavierjs/optimizeMatrixOps
Open

Optimize matrix inversions and reduce GPU stalls #7367
xavierjs wants to merge 15 commits intomaplibre:mainfrom
xavierjs:xavierjs/optimizeMatrixOps

Conversation

@xavierjs
Copy link
Copy Markdown
Contributor

@xavierjs xavierjs commented Apr 1, 2026

Hi,

Thank you for considering my PR.

Issue

To compute matrix inversion, we use the general method mat4.invert from the gl-matrix package: https://github.com/toji/gl-matrix/blob/master/src/mat4.js#L293
This function compute the invert matrix using the general auto-adjoint formulae.

Proposed solution

In most cases, the matrix to invert has a specific format (transformation matrix, projection matrix, ...), so we can compute the inverse with far less operations.

In this PR, we also avoid some useless allocations, and we add 2 minor optimizations:

  • to test if we are running a WebGL2 context, we just run gl instanceof WebGL2RenderingContext;
  • we avoid calling gl.getParameter(gl.MAX_TEXTURE_SIZE); at each raster tile loading (in general gl.getParameter can stall the GPU for some configuration, we should cache its value when possible)

Benchmarks

The performance improvement is not drastic. There is a 7% gain on the Placement benchmark.

Misc

No related issues.
This PR does not include any visual changes and does not require additional tests.
npm run test-renderruns exactly as before.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.79%. Comparing base (7f61f6f) to head (c7a6fb9).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7367      +/-   ##
==========================================
+ Coverage   92.77%   92.79%   +0.01%     
==========================================
  Files         289      290       +1     
  Lines       24017    24083      +66     
  Branches     5100     5099       -1     
==========================================
+ Hits        22282    22348      +66     
  Misses       1735     1735              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@HarelM
Copy link
Copy Markdown
Collaborator

HarelM commented Apr 1, 2026

Thanks for taking the time to open this PR!
Are you sure the fast matrix inversion is safe and won't cause issue on some edge cases?
Should there be a check to make sure a matrix is "of the right type" before applying the fast inversion? I'm asking to make sure this was given the right thought not because I think it has to be there.

@xavierjs
Copy link
Copy Markdown
Contributor Author

xavierjs commented Apr 6, 2026

Thanks for taking the time to open this PR! Are you sure the fast matrix inversion is safe and won't cause issue on some edge cases? Should there be a check to make sure a matrix is "of the right type" before applying the fast inversion? I'm asking to make sure this was given the right thought not because I think it has to be there.

I would be difficult to test the matrix type because a full test may be as costly as using the adjoint matrix formulae. Where we use specific matrix inversion there is already strong hypothesis on the matrix format (like we know it is and it should be a projection matrix, or a transform matrix, ...).

@HarelM
Copy link
Copy Markdown
Collaborator

HarelM commented Apr 6, 2026

Ok, thanks. Please make sure to address the other code review comments.

@xavierjs
Copy link
Copy Markdown
Contributor Author

xavierjs commented Apr 6, 2026

Ok, thanks. Please make sure to address the other code review comments.

I think I implemented feedback or answered to all comments in this PR.

@HarelM
Copy link
Copy Markdown
Collaborator

HarelM commented Apr 6, 2026

There are still two open comments that were not addressed and no replay, have you forgot to push some code?
In any case, there are conflicts to resolve before this can be merged.

@HarelM
Copy link
Copy Markdown
Collaborator

HarelM commented Apr 9, 2026

Added a last comment, thanks for the improved description.

@xavierjs
Copy link
Copy Markdown
Contributor Author

I did some mess with a rebase, I am fixing it

@xavierjs xavierjs force-pushed the xavierjs/optimizeMatrixOps branch from 4d7df8b to 1bf0984 Compare April 13, 2026 13:46
@xavierjs xavierjs requested a review from HarelM April 13, 2026 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants