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

GPU Expressions #2202

Merged
merged 43 commits into from
May 17, 2024
Merged

GPU Expressions #2202

merged 43 commits into from
May 17, 2024

Conversation

TimSylvester
Copy link
Collaborator

@TimSylvester TimSylvester commented Mar 15, 2024

We would like to eventually move as much of the style property evaluation as is practical to the shaders. This sets up some foundational aspects of that and allows basic zoom-based expressions in line layers to be evaluated in shaders.

The legacy code used uniforms for these same zoom-based expressions, updating them on each frame. Unfortunately, the entire UBO still needs to be updated if any of the properties are CPU-evaluated, so we don't really gain any efficiency from the binding or data transfer. We skip some of the actual property evaluation, but that barely shows up in profiling.

To be really beneficial, we will need to get to the point where we can use shaders for per-attribute expressions, so that the much larger vertex attribute arrays do not need to be updated constantly. That will involve making the feature data available to the shaders with some pre-processing.

Currently only applies to non-custom line layers.

@TimSylvester TimSylvester self-assigned this Mar 15, 2024
Copy link

github-actions bot commented Mar 15, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +1.2%  +210Ki  +1.2%  +208Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2202-compared-to-main.txt

Copy link

github-actions bot commented Mar 18, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.3%  +435Ki  +0.2% +68.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2202-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +21% +24.2Mi  +409% +24.4Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2202-compared-to-legacy.txt

@TimSylvester TimSylvester force-pushed the expr-deps-2 branch 3 times, most recently from 5681621 to dba57fa Compare March 28, 2024 22:31
@TimSylvester TimSylvester force-pushed the expr-deps-2 branch 2 times, most recently from 4fc8155 to 10a4b7c Compare April 3, 2024 21:54
@TimSylvester TimSylvester marked this pull request as ready for review April 3, 2024 21:57
Copy link

github-actions bot commented Apr 3, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0049         +0.0053             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2202-compared-to-main.txt

Copy link
Collaborator

@sjg-wdw sjg-wdw left a comment

Choose a reason for hiding this comment

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

Looks good to me.

louwers
louwers previously requested changes Apr 4, 2024
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

The pre-commit hook tried to format the code, but it could not push because the fork is owned by an organization. You can run pre-commit locally with:

pre-commit run --all-files

Could you update the PR description to include a description of the changes made to the iOS Benchmark app?

The PR title could be a bit more descriptive. This is not the first PR on GPU Expressions, and I am assuming it won't be the last. If you hover over a piece of code or use git blame it's nice to get a short description with a summary to know as part of what effort the code in question was added.

A few more details about the implementation and the reason behind them in the description would also be appreciated. Each PR should ideally have a self-contained description of the changes.

Other than that, code looks good. Thanks!

@TimSylvester
Copy link
Collaborator Author

@louwers I have pre-commit set up to run on commits, and running with --all-files changes a bunch of files that I haven't interacted with, e.g., bin/offline.cpp. It would be better to to apply all those changes to main first, rather than including them here, I think.

@louwers
Copy link
Collaborator

louwers commented Apr 9, 2024

@TimSylvester Sure. I'll make a PR.

Edit: I don't see any changes to bin/offline.cpp. #2245

@TimSylvester
Copy link
Collaborator Author

Updated benchmark, pretty much the same.

CPU:

Average frame encoding time: 2.78 ms
Average frame rendering time: 6.55 ms

Average frame encoding time: 2.77 ms
Average frame rendering time: 6.47 ms

Average frame encoding time: 2.79 ms
Average frame rendering time: 6.57 ms

GPU:

Average frame encoding time: 2.76 ms
Average frame rendering time: 6.70 ms

Average frame encoding time: 2.79 ms
Average frame rendering time: 6.77 ms

Average frame encoding time: 2.76 ms
Average frame rendering time: 6.76 ms

So, a small hit in render time with no real benefit to encoding time. The CPU work we're avoiding is not very costly, and we can't avoid updating and binding the evaluated property UBO unless every single property could be done on the GPU.

@TimSylvester TimSylvester requested review from louwers and sjg-wdw May 1, 2024 20:24
@TimSylvester TimSylvester merged commit 33a1e16 into maplibre:main May 17, 2024
32 of 34 checks passed
@TimSylvester TimSylvester deleted the expr-deps-2 branch May 17, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants