-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Performance improvement and reduced shader workload with globe projection #12039
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
Conversation
6bbccbc
to
501c74e
Compare
@karimnaaji Any comments? Was the refactoring unfeasible? |
@mourner I might have accidentally closed this. It's still a good WIP and totally feasible, I'll pick that up again once benchmap has globe in the set of fixtures as it's related to improvement to the transition. |
ab160f0
to
d6b5668
Compare
Do not bind uniforms on empty location Fix unit tests Do not refetch unused uniform values
d6b5668
to
7fe3bb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice win here, looks great!
This PR aims to reduce the workload and improve performance during the
globe
-mercator
transition and initial map load. It currently does two kind of improvements:globe
.Reduced shader compilation scenario can be tested with the following diff by comparing against `main`
getUniformLocation
at first use. This prevents us from trying to upload uniforms on unused uniform locations and overall simplifies extra code necessary to instantiate them, which also reduces library size.The results from benchmap can be seen at: https://sites.mapbox.com/benchmap-js-results/stack/benchmap-karim-dev/runs/25. In summary, it improves the metrics
load time
by ~38ms,speed index
by ~5% andrender
andtotalBlockingTime
by 57ms and 40ms respectively on therendering-globe
fixture. It does not regress any other fixtures unrelated to globe.Other metrics improvement
Here is a before/after video taken from benchmap fixture to see the change and reduced stutter visually:
before-after.mov
There might still be room for improvement, but I kept the scope small so that it can be reviewed and eventual follow-ups investigated and addressed separately. Specifically, one follow-up investigation to consider could be to defer the compilation of shader variants that we know could be of use as a low priority task whenever the map will become idle.
As part of this PR, I also investigated whether we could reduce the number of redundant gl calls that we might have when drawing globes with regards to texture bindings. We currently allow to cache the texture globally, but we should instead cache per texture slot, since as long as a texture is bound to a slot it can be considered strongly associated with that slot. So once it is bound, any further request to bind the combination
texture slot
/texture id
is redundant.Redundant trace chunk
Note the frequent and unnecessary redundant bindings:
Eliminating these redundant calls from our traces did not yet result in a visible performance improvement (e09e055, https://sites.mapbox.com/benchmap-js-results/stack/benchmap-karim-dev/runs/17) so I didn't pursue this idea any further at the moment, and it might need more fine-grained profiling or more data samples.
Fixes https://github.com/mapbox/gl-js-team/issues/455
Launch Checklist
mapbox-gl-js
changelog:<changelog>Improve globe-mercator transition and map load performance with globe projection</changelog>