Skip to content

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

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.

Sign up for GitHub

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

Open
JoonghyunCho wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c157e20
[Tizen.NUI.Components] Remove temporary unrealizedItems list in Linea…
May 17, 2026
03a8955
Address review feedback
May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions src/Tizen.NUI.Components/Controls/RecyclerView/Layouter/GridLayouter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,17 +416,16 @@ public override void RequestLayout(float scrollPosition, bool force = false)
//Console.WriteLine("[NUI] {0} :visibleArea before [{1},{2}] after [{3},{4}]", scrollPosition, prevFirstVisible, prevLastVisible, FirstVisible, LastVisible);

// 2. Unrealize invisible items.
List<RecyclerViewItem> unrealizedItems = new List<RecyclerViewItem>();
foreach (RecyclerViewItem item in VisibleItems)
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
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(N×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.


//Console.WriteLine("Realize Begin [{0} to {1}]", FirstVisible, LastVisible);
// 3. Realize and placing visible items.
Expand Down Expand Up @@ -1029,22 +1028,20 @@ public override void NotifyItemRangeRemoved(IItemSource source, int startIndex,
else collectionView.ContentContainer.SizeHeight = ScrollContentSize;

// 3. Update Visible Items.
List<RecyclerViewItem> unrealizedItems = new List<RecyclerViewItem>();
foreach (RecyclerViewItem item in VisibleItems)
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;
});

// Position Adjust
float scrollPosition = PrevScrollPosition;
Expand Down
22 changes: 9 additions & 13 deletions src/Tizen.NUI.Components/Controls/RecyclerView/Layouter/LinearLayouter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,17 +419,15 @@ public override void RequestLayout(float scrollPosition, bool force = false)
LastVisible = end;

// 2. Unrealize invisible items.
List<RecyclerViewItem> unrealizedItems = new List<RecyclerViewItem>();
foreach (RecyclerViewItem item in VisibleItems)
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.


// 3. Realize and placing visible items.
for (int i = FirstVisible; i <= LastVisible; i++)
Expand Down Expand Up @@ -1134,22 +1132,20 @@ public override void NotifyItemRangeRemoved(IItemSource source, int startIndex,
else collectionView.ContentContainer.SizeHeight = ScrollContentSize;

// 4. Update Visible Items.
List<RecyclerViewItem> unrealizedItems = new List<RecyclerViewItem>();
foreach (RecyclerViewItem item in VisibleItems)
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.


if (startIndex <= FirstVisible)
{
Expand Down
Loading