Refactor: Reuse OutlineFilters to fix allocation issue and potential GC overhead in StructureIconsLayer#3339
Refactor: Reuse OutlineFilters to fix allocation issue and potential GC overhead in StructureIconsLayer#3339Skigim wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
WalkthroughReplaces inline Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
didn't have time to look deeper, but from a quick glance: |
So from what I can tell, the pixi library builds a programCache that covers the GPU side, but the js objects aren't cached on the CPU end so GC is still getting a workout with the memory overload. I'll run profiler when I get home to see if there's a substantial benefit - if not I'll edit title to refactor. |
Skigim
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
335-365: This still creates new filter arrays in the hot path.The
OutlineFilterobjects are reused now, but eachfilters = [FILTER_OUTLINE_*]here still allocates a fresh array, and the repeated writes can keep some GC pressure in the exact path this PR is trying to cool down. A small helper that skips no-op assignments and reuses the common filter-array values would make the perf win more complete.Also applies to: 640-641
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureIconsLayer.ts` around lines 335 - 365, The code repeatedly assigns new one-element arrays to sprite.container.filters (e.g., this.ghostUnit.container.filters = [FILTER_OUTLINE_RED] and similar for FILTER_OUTLINE_GREEN), causing garbage churn; add reusable const arrays like FILTERS_OUTLINE_RED = [FILTER_OUTLINE_RED] and FILTERS_OUTLINE_GREEN = [FILTER_OUTLINE_GREEN] and a small helper method (e.g., setFilters(container: PIXI.Container, filters: readonly any[])) that checks if container.filters === filters and only assigns when different, then replace all inline assignments in StructureIconsLayer (including the ghostUnit, potentialUpgrade.iconContainer, potentialUpgrade.dotContainer and the other occurrences mentioned) to call setFilters with the shared arrays to avoid allocating fresh arrays on the hot path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 335-365: The code repeatedly assigns new one-element arrays to
sprite.container.filters (e.g., this.ghostUnit.container.filters =
[FILTER_OUTLINE_RED] and similar for FILTER_OUTLINE_GREEN), causing garbage
churn; add reusable const arrays like FILTERS_OUTLINE_RED = [FILTER_OUTLINE_RED]
and FILTERS_OUTLINE_GREEN = [FILTER_OUTLINE_GREEN] and a small helper method
(e.g., setFilters(container: PIXI.Container, filters: readonly any[])) that
checks if container.filters === filters and only assigns when different, then
replace all inline assignments in StructureIconsLayer (including the ghostUnit,
potentialUpgrade.iconContainer, potentialUpgrade.dotContainer and the other
occurrences mentioned) to call setFilters with the shared arrays to avoid
allocating fresh arrays on the hot path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 508f6847-9e59-4f54-9678-8f3234af88e4
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts
Skigim
changed the title
# Conflicts: # src/client/graphics/layers/StructureIconsLayer.ts
Skigim
changed the title
There was a problem hiding this comment.
Pull request overview
Refactors StructureIconsLayer.ts to reduce late-game GC churn by reusing pixi-filters OutlineFilter instances instead of repeatedly allocating new ones during ghost/visibility updates.
Changes:
- Introduces shared red/green/white
OutlineFilterconstants initialized once at module load. - Replaces per-use
new OutlineFilter(...)allocations with references to the shared filter instances.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
350-380: You still allocate fresh filter arrays in the hot branches.The filter objects are reused now, but each
[FILTER_OUTLINE_*]still creates a short-lived array. If profiling still shows GC here, hoisting the one-item arrays too would remove the last new allocations from these assignments.♻️ Possible follow-up
const FILTER_OUTLINE_RED = new OutlineFilter({ thickness: 2, color: "rgba(255, 0, 0, 1)", }); const FILTER_OUTLINE_GREEN = new OutlineFilter({ thickness: 2, color: "rgba(0, 255, 0, 1)", }); const FILTER_OUTLINE_WHITE = new OutlineFilter({ thickness: 2, color: "rgb(255, 255, 255)", }); +const FILTERS_OUTLINE_RED = [FILTER_OUTLINE_RED]; +const FILTERS_OUTLINE_GREEN = [FILTER_OUTLINE_GREEN]; +const FILTERS_OUTLINE_WHITE = [FILTER_OUTLINE_WHITE];- this.ghostUnit.container.filters = [FILTER_OUTLINE_RED]; + this.ghostUnit.container.filters = FILTERS_OUTLINE_RED; … - this.potentialUpgrade.iconContainer.filters = [ - FILTER_OUTLINE_GREEN, - ]; - this.potentialUpgrade.dotContainer.filters = [FILTER_OUTLINE_GREEN]; + this.potentialUpgrade.iconContainer.filters = FILTERS_OUTLINE_GREEN; + this.potentialUpgrade.dotContainer.filters = FILTERS_OUTLINE_GREEN; … - this.ghostUnit.container.filters = [FILTER_OUTLINE_RED]; + this.ghostUnit.container.filters = FILTERS_OUTLINE_RED; … - render.iconContainer.filters = [FILTER_OUTLINE_WHITE]; - render.dotContainer.filters = [FILTER_OUTLINE_WHITE]; + render.iconContainer.filters = FILTERS_OUTLINE_WHITE; + render.dotContainer.filters = FILTERS_OUTLINE_WHITE;Also applies to: 694-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureIconsLayer.ts` around lines 350 - 380, The code still creates new one-item filter arrays inline (e.g., assignments like this.ghostUnit.container.filters = [FILTER_OUTLINE_RED], this.potentialUpgrade.iconContainer.filters = [FILTER_OUTLINE_GREEN], and dotContainer.filters = [FILTER_OUTLINE_GREEN]), causing short-lived allocations; fix by hoisting these one-item arrays into reusable constants (e.g., OUTLINE_RED_FILTER_ARRAY, OUTLINE_GREEN_FILTER_ARRAY) and use those constants in StructureIconsLayer where you set container.filters and dotContainer.filters (also update the similar occurrences around the other noted location at lines 694-695) so the same array instances are reused instead of allocating each time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 350-380: The code still creates new one-item filter arrays inline
(e.g., assignments like this.ghostUnit.container.filters = [FILTER_OUTLINE_RED],
this.potentialUpgrade.iconContainer.filters = [FILTER_OUTLINE_GREEN], and
dotContainer.filters = [FILTER_OUTLINE_GREEN]), causing short-lived allocations;
fix by hoisting these one-item arrays into reusable constants (e.g.,
OUTLINE_RED_FILTER_ARRAY, OUTLINE_GREEN_FILTER_ARRAY) and use those constants in
StructureIconsLayer where you set container.filters and dotContainer.filters
(also update the similar occurrences around the other noted location at lines
694-695) so the same array instances are reused instead of allocating each time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82164b00-f787-49d1-a67c-15ec2b419337
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts
This PR is the second slice of updates addressing #3207 by optimizing how we draw visual outlines for structures in
StructureIconsLayer.ts.The Problem:
Currently we create new outline filters instead of reusing them, which adds unnecessary allocations and some extra GC work.
The Fix:
The goal is for this to fix aforementioned allocation issue in [StructureIconsLayer] by reusing [OutlineFilter] instances instead.
In theory that should help performance a little by cutting some unnecessary allocation work, but the effect is small enough that I could not measure it reliably with browser profiling.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
skigim