Bugfix/merged optimizations #578
Conversation
…s in source tile data manager on update, allow more gl tasks on pre-render (within given time constraint), don't cache zoom-dependent values (prevent cache overflow)
WalkthroughThe updates across various files in an Android graphics-related codebase include conditional OpenGL buffer generation, adjusted rendering intervals based on timing, and enhanced tile loading in a mapping source. Additionally, there have been refinements in value evaluation logic and changes to symbol object instance count return types, suggesting an overall improvement in graphics rendering efficiency and data handling. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (8)
- android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp (1 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/graphics/GLThread.kt (1 hunks)
- shared/public/Tiled2dMapSource.h (2 hunks)
- shared/public/Tiled2dMapSourceImpl.h (1 hunks)
- shared/public/Value.h (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1 hunks)
Additional comments: 9
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1)
- 61-61: The
getInstanceCountsmethod's return type has been changed to a non-const value.This change is acceptable if the intention is to allow modification of the returned
SymbolObjectInstanceCountsobject.android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp (1)
- 105-111: Conditional OpenGL buffer generation logic has been added.
This is a good optimization to prevent unnecessary buffer generation.
android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp (1)
- 104-111: Conditional OpenGL buffer generation logic has been added.
This is a good optimization to prevent unnecessary buffer generation.
android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp (2)
- 106-112: The addition of new OpenGL buffers is consistent with the PR objectives to optimize the OpenGL rendering process. However, ensure that the rest of the codebase is updated to utilize these new buffers, and that any necessary cleanup is performed to remove old buffer usage if it has been replaced by this new implementation.
- 106-112: The code correctly checks if the OpenGL data buffers have been generated before attempting to generate them again. This is a good performance optimization to prevent unnecessary buffer generation.
shared/public/Tiled2dMapSourceImpl.h (4)
- 364-380: The implementation of debouncing logic for new tile loading is a good practice to avoid unnecessary loading tasks when the visible tiles change rapidly. However, ensure that the
PENDING_NEW_TILE_DEBOUNCE_MSconstant is defined and has a reasonable value to balance responsiveness with performance.Verification successful
The constant
PENDING_NEW_TILE_DEBOUNCE_MSis defined inTiled2dMapSource.hwith a value of 200 milliseconds, which is a reasonable value for debouncing tile loading in a UI context.* 389-398: The method `onStableTriggerNewTileLoading` is correctly checking if the update ID matches the current update ID before performing loading tasks. This ensures that only the latest update triggers the loading of new tiles, which is consistent with the debouncing logic. * 364-380: The use of a lambda function for debouncing tile loading is a good practice. However, it's important to ensure that the `LambdaTask` and `TaskConfig` are properly implemented and that the `scheduler` is capable of handling these tasks efficiently.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that PENDING_NEW_TILE_DEBOUNCE_MS is defined and check its value. rg --type cpp "PENDING_NEW_TILE_DEBOUNCE_MS"Length of output: 307
Verification successful
* 364-380: The locking of `mailbox->receivingMutex` before sorting and potentially updating `pendingTilesToLoad` is a good practice to prevent data races in a multithreaded environment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of LambdaTask and TaskConfig, and ensure the scheduler can handle these tasks. rg --type cpp "LambdaTask|TaskConfig"Length of output: 12176
Script:
#!/bin/bash # Verify the implementation details of the scheduler, specifically ThreadPoolSchedulerImpl rg --type cpp "ThreadPoolSchedulerImpl"Length of output: 2671
Script:
#!/bin/bash # Verify the implementation details of the methods provided by ThreadPoolSchedulerImpl to understand task management rg --type cpp -A 10 "void ThreadPoolSchedulerImpl::addTask" "shared/src/map/scheduling/ThreadPoolSchedulerImpl.cpp"Length of output: 1597
| auto result = value->evaluateOr(context, defaultValue); | ||
| return result; |
There was a problem hiding this comment.
The ValueEvaluator class has been modified to assign the result of value->evaluateOr(context, defaultValue) to a local variable result before returning it. This change is redundant as the result is not used for any processing and is immediately returned. This adds unnecessary lines of code and may slightly impact performance due to the additional variable assignment.
- auto result = value->evaluateOr(context, defaultValue);
- return result;
+ return value->evaluateOr(context, defaultValue);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| auto result = value->evaluateOr(context, defaultValue); | |
| return result; | |
| return value->evaluateOr(context, defaultValue); |
| @@ -164,8 +164,9 @@ class Tiled2dMapSource : | |||
| void onVisibleTilesChanged(const std::vector<VisibleTilesLayer> &pyramid, int keepZoomLevelOffset = 0); | |||
|
|
|||
| private: | |||
| void performLoadingTask(Tiled2dMapTileInfo tile, size_t loaderIndex); | |||
| void onStableTriggerNewTileLoading(uint64_t updateId); | |||
There was a problem hiding this comment.
The method onStableTriggerNewTileLoading has been added but lacks documentation.
Please add documentation to explain the purpose and usage of this method.
| @@ -181,11 +182,14 @@ | |||
|
|
|||
| std::unordered_map<size_t, std::map<Tiled2dMapTileInfo, ErrorInfo>> errorTiles; | |||
| std::optional<long long> nextDelayTaskExecution; | |||
| std::vector<PrioritizedTiled2dMapTileInfo> pendingTilesToLoad; | |||
There was a problem hiding this comment.
The member pendingTilesToLoad has been added but lacks documentation.
Please add documentation to explain the purpose and usage of this member.
|
|
||
| std::unordered_set<Tiled2dMapTileInfo> notFoundTiles; | ||
|
|
||
| std::string layerName; | ||
|
|
||
| uint64_t currentNewTilesUpdateId = 0; |
There was a problem hiding this comment.
The member currentNewTilesUpdateId has been added but lacks documentation.
Please add documentation to explain the purpose and usage of this member.
|
|
||
| std::unordered_set<Tiled2dMapTileInfo> notFoundTiles; | ||
|
|
||
| std::string layerName; | ||
|
|
||
| uint64_t currentNewTilesUpdateId = 0; | ||
| const static int64_t PENDING_NEW_TILE_DEBOUNCE_MS = 200; |
There was a problem hiding this comment.
The constant PENDING_NEW_TILE_DEBOUNCE_MS has been added but lacks documentation.
Please add documentation to explain the purpose and usage of this constant.
| void removeFromCache(); | ||
|
|
There was a problem hiding this comment.
Duplicate removeFromCache method definition.
- void removeFromCache();Remove the duplicate method definition to prevent compilation errors.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| void removeFromCache(); |
| @@ -127,6 +127,7 @@ class GLThread constructor( | |||
| hasFinishedSinceDirty = true | |||
| finishDuration = System.currentTimeMillis() - finishDuration | |||
| } | |||
| val finishDuration = System.currentTimeMillis() - preFinish | |||
There was a problem hiding this comment.
The variable finishDuration is redeclared, which will cause a compilation error in Kotlin.
- val finishDuration = System.currentTimeMillis() - preFinishRemove the redeclaration of finishDuration to use the previously declared variable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| val finishDuration = System.currentTimeMillis() - preFinish |
| if (!glDataBuffersGenerated) { | ||
| glGenBuffers(1, &positionsBuffer); | ||
| glGenBuffers(1, &textureCoordinatesListBuffer); | ||
| glGenBuffers(1, &scalesBuffer); | ||
| glGenBuffers(1, &rotationsBuffer); | ||
| glGenBuffers(1, &alphasBuffer); | ||
| glGenBuffers(1, &stretchInfoBuffer); |
There was a problem hiding this comment.
The conditional block for generating OpenGL buffers only if they haven't been generated before is a good practice to avoid redundant operations. However, there is no corresponding flag set to true for glDataBuffersGenerated after the buffers are generated. This could lead to the buffers not being generated at all if glDataBuffersGenerated is not properly initialized to false.
if (!glDataBuffersGenerated) {
glGenBuffers(1, &positionsBuffer);
glGenBuffers(1, &textureCoordinatesListBuffer);
glGenBuffers(1, &scalesBuffer);
glGenBuffers(1, &rotationsBuffer);
glGenBuffers(1, &alphasBuffer);
glGenBuffers(1, &stretchInfoBuffer);
glDataBuffersGenerated = true; // This line is missing to indicate that the buffers have been generated.
}
Summary by CodeRabbit
Performance Improvements
New Features
Refactor
ValueEvaluatorfor more consistent value processing.Tiled2dMapVectorSymbolObjectwith method return type refinement and removed duplicate code.