perf: Avoid redundant font map lookups#933
Conversation
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)lib/GfxRenderer/GfxRenderer.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (9)
📝 WalkthroughWalkthroughRefactors font-map lookups in GfxRenderer.cpp: replaces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/GfxRenderer/GfxRenderer.cpp (1)
749-757:⚠️ Potential issue | 🟡 MinorIncomplete refactor:
fontMap.at()still used afterfind().Line 756 still uses
fontMap.at(fontId)instead offontIt->second, which defeats the optimization goal by performing a redundant lookup.Proposed fix
return fontMap.at(fontId).getData(EpdFontFamily::REGULAR)->advanceY; + return fontIt->second.getData(EpdFontFamily::REGULAR)->advanceY;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/GfxRenderer/GfxRenderer.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
lib/GfxRenderer/GfxRenderer.cpp (1)
lib/Utf8/Utf8.cpp (2)
utf8NextCodepoint(11-31)utf8NextCodepoint(11-11)
🔇 Additional comments (7)
lib/GfxRenderer/GfxRenderer.cpp (7)
77-84: LGTM!Clean refactor from
count()+at()tofind()+ iterator reuse. The iterator is correctly checked againstend()before dereferencing.
104-109: LGTM!Good use of a const reference (
const auto& font = fontIt->second) to cache the map value for multiple subsequent uses.
714-720: LGTM!Correct refactor pattern applied.
724-734: LGTM!Good pattern: caches the font reference before the loop to avoid repeated iterator dereferences.
740-746: LGTM!Correct refactor pattern applied.
760-765: LGTM!Correct refactor pattern applied.
775-781: LGTM!Correct refactor with reference caching for the font used throughout the method.
## Summary **What is the goal of this PR?** Several methods in GfxRenderer were doing a `count()` followed by `at()` on the fonts map, effectively doing the same map lookup unnecessarily. This can be avoided by doing a single `find()` and reusing the iterator. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_
## Summary **What is the goal of this PR?** Several methods in GfxRenderer were doing a `count()` followed by `at()` on the fonts map, effectively doing the same map lookup unnecessarily. This can be avoided by doing a single `find()` and reusing the iterator. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_
## Summary **What is the goal of this PR?** Several methods in GfxRenderer were doing a `count()` followed by `at()` on the fonts map, effectively doing the same map lookup unnecessarily. This can be avoided by doing a single `find()` and reusing the iterator. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_
## Summary **What is the goal of this PR?** Several methods in GfxRenderer were doing a `count()` followed by `at()` on the fonts map, effectively doing the same map lookup unnecessarily. This can be avoided by doing a single `find()` and reusing the iterator. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_
## Summary **What is the goal of this PR?** Several methods in GfxRenderer were doing a `count()` followed by `at()` on the fonts map, effectively doing the same map lookup unnecessarily. This can be avoided by doing a single `find()` and reusing the iterator. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_
2 participants
Summary
What is the goal of this PR?
Several methods in GfxRenderer were doing a
count()followed byat()on the fonts map, effectively doing the same map lookup unnecessarily. This can be avoided by doing a singlefind()and reusing the iterator.AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? NO