×ばつM) RemoveAll(list.Contains) pattern from four scroll/range-removed sites in LinearLayouter and GridLayouter..." /> ×ばつM) RemoveAll(list.Contains) pattern from four scroll/range-removed sites in LinearLayouter and GridLayouter. The un..." />×ばつM) RemoveAll(list.Contains) pattern from four scroll/range-removed sites in LinearLayouter and GridLayouter. The un..." />
Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[AI Task] [Tizen.NUI.Components] Remove unrealizedItems temp list in Linear/GridLayouter scroll path#7644

Open
JoonghyunCho wants to merge 2 commits into
main from
ai-task/issue-7608
Open

[AI Task] [Tizen.NUI.Components] Remove unrealizedItems temp list in Linear/GridLayouter scroll path #7644
JoonghyunCho wants to merge 2 commits into
main from
ai-task/issue-7608

Conversation

@JoonghyunCho

@JoonghyunCho JoonghyunCho commented May 17, 2026

Copy link
×ばつM) `RemoveAll(list.Contains)` pattern from four scroll/range-removed sites in `LinearLayouter` and `GridLayouter`. The unrealize check is inlined directly into the `RemoveAll` predicate, giving a single O(N) pass with zero per-call allocation on the scroll hot path. ## Changes - `src/Tizen.NUI.Components/Controls/RecyclerView/Layouter/LinearLayouter.cs` - `RequestLayout` (line ~422): replaced foreach-into-temp-list + `RemoveAll(list.Contains)` with inline-predicate `RemoveAll`. - `NotifyItemRangeRemoved` (line ~1137): same replacement; the `item.Index -= count` shift is folded into the same predicate. - `src/Tizen.NUI.Components/Controls/RecyclerView/Layouter/GridLayouter.cs` - `RequestLayout` (line ~419): same replacement. - `NotifyItemRangeRemoved` (line ~1032): same replacement. ## Mode Refactoring ## Performance impact (per call) | Item | Before | After | |---|---|---| | `List<>` allocation | 1 + backing array | 0 | | `RemoveAll` predicate | O(N ×ばつ M) | O(N) | | Total passes over `VisibleItems` | 2 (foreach + RemoveAll) | 1 | `RequestLayout` is called many times per second on scroll, so this directly removes per-frame Gen0 pressure on the recycler hot path. ## API compatibility - Public API signatures: unchanged. - Behavior: unchanged. The predicate observes each element exactly once and performs the same unrealize + (in the range-removed path) the same `Index -= count` shift for items above the removed range. - No native interop change. ## Verification - [x] Build: passed (`dotnet build src/Tizen.NUI.Components` — 0 errors; pre-existing warnings unchanged). - [x] Tests: N/A (no unit tests cover these specific internal paths; behavior is preserved by construction). - [x] Benchmark: skipped (no `sdb` device available in this environment — manual on-device benchmark recommended). Fixes #7608 " data-view-component="true"> Copy Markdown
Member

Summary

Removes the temporary List<RecyclerViewItem> allocation and the O(×ばつM) RemoveAll(list.Contains) pattern from four scroll/range-removed sites in LinearLayouter and GridLayouter. The unrealize check is inlined directly into the RemoveAll predicate, giving a single O(N) pass with zero per-call allocation on the scroll hot path.

Changes

  • src/Tizen.NUI.Components/Controls/RecyclerView/Layouter/LinearLayouter.cs
    • RequestLayout (line ~422): replaced foreach-into-temp-list + RemoveAll(list.Contains) with inline-predicate RemoveAll.
    • NotifyItemRangeRemoved (line ~1137): same replacement; the item.Index -= count shift is folded into the same predicate.
  • src/Tizen.NUI.Components/Controls/RecyclerView/Layouter/GridLayouter.cs
    • RequestLayout (line ~419): same replacement.
    • NotifyItemRangeRemoved (line ~1032): same replacement.

Mode

Refactoring

Performance impact (per call)

Item Before After
List<> allocation 1 + backing array 0
RemoveAll predicate O(N ×ばつ M) O(N)
Total passes over VisibleItems 2 (foreach + RemoveAll) 1

RequestLayout is called many times per second on scroll, so this directly removes per-frame Gen0 pressure on the recycler hot path.

API compatibility

  • Public API signatures: unchanged.
  • Behavior: unchanged. The predicate observes each element exactly once and performs the same unrealize + (in the range-removed path) the same Index -= count shift for items above the removed range.
  • No native interop change.

Verification

  • Build: passed (dotnet build src/Tizen.NUI.Components — 0 errors; pre-existing warnings unchanged).
  • Tests: N/A (no unit tests cover these specific internal paths; behavior is preserved by construction).
  • Benchmark: skipped (no sdb device available in this environment — manual on-device benchmark recommended).

Fixes #7608

gemini-code-assist[bot] reacted with eyes emoji 
...r/GridLayouter
Inline the unrealize check directly into VisibleItems.RemoveAll predicate
in four scroll hot-path / NotifyItemRangeRemoved sites:
- LinearLayouter.cs (RequestLayout, NotifyItemRangeRemoved)
- GridLayouter.cs (RequestLayout, NotifyItemRangeRemoved)
Before: allocated a temporary List<RecyclerViewItem> per call and used
RemoveAll(list.Contains) (O(N*M) linear scan on each predicate call).
After: single O(N) RemoveAll pass with zero allocation per call.
No public API change. Behavior preserved.
Fixes #7608

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes the removal of invisible or removed items in GridLayouter and LinearLayouter by replacing temporary list allocations and O(N*M) lookups with the RemoveAll method. While this improves performance to O(N), the review highlights that the current implementation still incurs heap allocations due to closure captures in the lambdas. To achieve true zero allocation on these performance-critical paths, a manual iteration and shifting approach was suggested.

Comment on lines +419 to +428
VisibleItems.RemoveAll(item =>
{
if (item.Index < FirstVisible || item.Index > LastVisible)
{
//Console.WriteLine("[NUI] Unrealize{0}!", item.Index);
unrealizedItems.Add(item);
collectionView.UnrealizeItem(item);
return true;
}
}
VisibleItems.RemoveAll(unrealizedItems.Contains);
return false;
});

@gemini-code-assist gemini-code-assist Bot May 17, 2026

Copy link
×ばつM) to O(N), it doesn't strictly achieve "zero per-call allocation" as stated in the PR description. In C#, a lambda that captures local variables or `this` (which happens here via `FirstVisible`, `LastVisible`, and `collectionView`) results in a heap allocation for a closure object and a delegate instance on every call. Since `RequestLayout` is on the scroll hot path, you could achieve true zero allocation by using a manual `for` loop to shift elements in-place, followed by a `RemoveRange` call. However, `RemoveAll` is much more idiomatic and readable, so this is only worth changing if absolute zero allocation is required for this performance-critical path." data-view-component="true"> Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While this change significantly improves performance by moving from O(×ばつM) to O(N), it doesn't strictly achieve "zero per-call allocation" as stated in the PR description. In C#, a lambda that captures local variables or this (which happens here via FirstVisible, LastVisible, and collectionView) results in a heap allocation for a closure object and a delegate instance on every call.

Since RequestLayout is on the scroll hot path, you could achieve true zero allocation by using a manual for loop to shift elements in-place, followed by a RemoveRange call. However, RemoveAll is much more idiomatic and readable, so this is only worth changing if absolute zero allocation is required for this performance-critical path.

Comment on lines +422 to +430
VisibleItems.RemoveAll(item =>
{
if (item.Index < FirstVisible || item.Index > LastVisible)
{
unrealizedItems.Add(item);
collectionView.UnrealizeItem(item);
return true;
}
}
VisibleItems.RemoveAll(unrealizedItems.Contains);
unrealizedItems.Clear();
return false;
});

@gemini-code-assist gemini-code-assist Bot May 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the change in GridLayouter, this RemoveAll call involves a closure allocation because the lambda captures state. To completely eliminate heap pressure on the scroll hot path, a manual iteration and shifting approach would be required. Given the PR's focus on eliminating allocations, this is a relevant detail to consider.

Comment on lines +1135 to +1148
VisibleItems.RemoveAll(item =>
{
if ((item.Index >= startIndex)
&& (item.Index < startIndex + count))
{
unrealizedItems.Add(item);
collectionView.UnrealizeItem(item);
return true;
}
else if (item.Index >= startIndex + count)
if (item.Index >= startIndex + count)
{
item.Index -= count;
}
}
VisibleItems.RemoveAll(unrealizedItems.Contains);
unrealizedItems.Clear();
return false;
});

@JoonghyunCho JoonghyunCho May 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI Review]
🟡 Suggestion: The lambda still captures startIndex, count, and collectionView, forcing a per-call closure allocation — so the PR's stated "zero per-call allocation" goal is not fully achieved here. A reverse-iteration for loop with RemoveAt eliminates both the temp list and the closure with no behavior change.

Suggested change
VisibleItems.RemoveAll(item =>
{
if ((item.Index >= startIndex)
&& (item.Index < startIndex + count))
{
unrealizedItems.Add(item);
collectionView.UnrealizeItem(item);
return true;
}
else if (item.Index >= startIndex + count)
if (item.Index >= startIndex + count)
{
item.Index -= count;
}
}
VisibleItems.RemoveAll(unrealizedItems.Contains);
unrealizedItems.Clear();
return false;
});
for (int i = VisibleItems.Count - 1; i >= 0; i--)
{
RecyclerViewItem item = VisibleItems[i];
if (item.Index >= startIndex && item.Index < startIndex + count)
{
collectionView.UnrealizeItem(item);
VisibleItems.RemoveAt(i);
}
else if (item.Index >= startIndex + count)
{
item.Index -= count;
}
}

@JoonghyunCho JoonghyunCho May 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI Review]
Addressed in 03a8955 — replaced RemoveAll(lambda) with a reverse for loop using RemoveAt, eliminating both the temp list and the closure allocation on this path.

 

Replace RemoveAll(lambda) with reverse for + RemoveAt in four Linear/GridLayouter
sites to eliminate the per-call closure allocation noted by reviewers, fully
realizing the PR's "zero per-call allocation" goal on the scroll hot path.
Applied-Human-Comments: 3254514563,3254514566
Applied-AI-Comments: 3255636218

Copy link
Copy Markdown
Member Author

🤖 [AI Review]
Addressed review feedback in commit 03a8955. Summary: replaced the four VisibleItems.RemoveAll(lambda) sites in LinearLayouter/GridLayouter (RequestLayout + NotifyItemRangeRemoved) with a reverse for + RemoveAt loop, eliminating the per-call closure allocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongsug-song dongsug-song Awaiting requested review from dongsug-song dongsug-song is a code owner

@bshsqa bshsqa Awaiting requested review from bshsqa bshsqa is a code owner

@hinohie hinohie Awaiting requested review from hinohie hinohie is a code owner

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

1 participant

AltStyle によって変換されたページ (->オリジナル) /