-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Change runtime error to the nullptr returning #3184
Conversation
TatyanaPolunina
commented
Jan 31, 2025
- We are attempting to create shaders during the update of layers. It seems more appropriate to return a nullptr for a shader that fails to create, rather than stopping the app if one shader is not created (as exceptions are not caught).
- Proper handling is implemented in all callers when a shader is nullptr, so we should not experience inconsistent behavior in such cases.
- Most shader creation errors are related to low memory on iOS. Since we have lazy initialization for shaders, the behavior may be restored in the next level update if iOS frees the necessary memory.
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.
Returning null works, but on each subsequent call of getOrCreateShader
the same shader creation will be retried and it can end up compiling the same code multiple times per frame.
An empty/debug "Null" shader should fix this, but I don't think the current checks can handle it.
Either way a performance hit is better than an unhandled exception.
Potentially it could be even better. As shader is not created, that's why we have Null. Also we usually have "compile error" together with "memory warning" on IOS, so I expect that on next "updateLayers" IOS could clear the memory and we would correctly create shaders so app would work. But need to be investigated for sure |
3ce1b5a
to
adc0c10
Compare
Bloaty Results (iOS) 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-3184-compared-to-main.txt |
Bloaty Results 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3184-compared-to-main.txtCompared to d387090 (legacy)
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3184-compared-to-legacy.txt |
Benchmark Results ⚡
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-3184-compared-to-main.txt |
…ptr handling for function callers
80f7a17
to
e460550
Compare