-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(core): Enable shader/pipeline caching for attached WebGL devices #9971
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
base: master
Are you sure you want to change the base?
fix(core): Enable shader/pipeline caching for attached WebGL devices #9971
Conversation
When attaching to an external WebGL context (e.g., MapboxOverlay with interleaved: true), the caching props were not being set, unlike the _createDevice() path which explicitly enables them. This caused severe performance degradation in interleaved mode because luma.gl's PipelineFactory would create new shader pipelines every frame instead of reusing cached ones. The fix adds _cacheShaders: true and _cachePipelines: true to the webgl2Adapter.attach() call, matching the defaults set in _createDevice(). User-provided deviceProps can still override these if needed. Fixes visgl#9839 Related: visgl#9822, luma.gl#2465
| // Enable shader and pipeline caching for attached devices (matches _createDevice defaults) | ||
| // Without this, interleaved mode (e.g., MapboxOverlay) creates new pipelines every frame | ||
| _cacheShaders: true, | ||
| _cachePipelines: true, |
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.
Duplicated caching defaults across device creation paths
Low Severity
The _cacheShaders: true and _cachePipelines: true default values are now duplicated in two separate locations: the webgl2Adapter.attach() call and the _createDevice() method. While the comment notes it "matches _createDevice defaults," this relies on manual synchronization. Extracting these defaults to a shared constant would ensure consistent behavior if defaults need to change.
Additional Locations (1)
chrisgervang
left a comment
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.
Thank you for submitting this PR - much appreciated! Left one comment. Please confirm there aren't any regressions to the other interleaved renderers - we now have a examples/basemap-browser app to help you quickly QA each integration
| // Enable shader and pipeline caching for attached devices (matches _createDevice defaults) | ||
| // Without this, interleaved mode (e.g., MapboxOverlay) creates new pipelines every frame | ||
| _cacheShaders: true, | ||
| _cachePipelines: true, |
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.
Let's put this below the spread in case a user wants to customize their device props. By default deviceProps is a {} so the caching will still apply
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.
Awesome, thank you so much @chrisgervang! That's a great catch: I've gone ahead and made the required change. I've also gone ahead and manually verified the changes via the examples/basemap-browser that you had recommended, and can confirm that no regressions were found, having tested it across all integrations.
Just as a slight note for anyone else wanting to verify the change: I believe that the performance impact scales with scene complexity and interaction frequency. As the examples/basemap-browser are quite simple, the problem doesn't always seem apparent, though this quickly changes in production applications with greater complexity and data.
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.
Oh my apologies, I misread your code. Your original code allowed the user to customize the props and now it forces the caching. Let's revert to your original version, please.
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.
Good to hear the example is working!
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.
Hahaha, no worries! Changes reverted and pushed.
…ys applies. Co-authored-by: chrisgervang <[email protected]>
…vices' into fix/enable-caching-for-attached-devices
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
- Caching is enabled by default - User is able to override default cache-settings via deviceProps should they need.
…vices' into fix/enable-caching-for-attached-devices
felixpalmer
left a comment
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.
Thanks :)
When attaching to an external WebGL context (e.g., MapboxOverlay with interleaved: true), the caching props were not being set, unlike the _createDevice() path which explicitly enables them.
This caused severe performance degradation in interleaved mode because luma.gl's PipelineFactory would create new shader pipelines every frame instead of reusing cached ones.
The fix adds _cacheShaders: true and _cachePipelines: true to the webgl2Adapter.attach() call, matching the defaults set in _createDevice(). User-provided deviceProps can still override these if needed.
Closes #9839
Related: #9822, luma.gl#2465
Note
Low Risk
Small, localized change to WebGL device initialization defaults; main risk is unexpected interaction if consumers relied on caching being disabled when attaching to an external context.
Overview
Fixes a performance regression when
Deckattaches to an externally-managed WebGL2 context by explicitly enabling luma.gl shader/pipeline caching (_cacheShaders/_cachePipelines) in thewebgl2Adapter.attach()path.Also reorders
_createDevice()’sdevicePropsspreading so userdevicePropsare applied before these caching defaults, keeping behavior consistent across device creation paths.Written by Cursor Bugbot for commit 4665384. This will update automatically on new commits. Configure here.