Skip to content

[perf] hit testing and scroll #5

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
gmacmaster wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[perf] hit testing and scroll #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a27e80
Eliminate O(n²) hit testing by caching visual children for indexed ac…
gmacmaster Apr 17, 2026
280ac82
Skip snap scroll reconfiguration when snap points haven't changed
gmacmaster Apr 17, 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
36 changes: 26 additions & 10 deletions vnext/Microsoft.ReactNative/Fabric/Composition/CompositionContextHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "pch.h"
#include "CompositionContextHelper.h"
#include <algorithm>
#include <vector>
#if __has_include("Composition.Experimental.SystemCompositionContextHelper.g.cpp")
#include "Composition.Experimental.SystemCompositionContextHelper.g.cpp"
#endif
Expand Down Expand Up @@ -435,26 +436,33 @@ struct CompVisualImpl {
auto compVisual = TTypeRedirects::CompositionContextHelper::InnerVisual(visual);
if (index == 0) {
containerChildren.InsertAtBottom(compVisual);
return;
} else {
auto insertAfter = containerChildren.First();
for (uint32_t i = 1; i < index; i++)
insertAfter.MoveNext();
containerChildren.InsertAbove(compVisual, insertAfter.Current());
}
if (index >= m_childrenCache.size()) {
m_childrenCache.push_back(visual);
} else {
m_childrenCache.insert(m_childrenCache.begin() + index, visual);
}
auto insertAfter = containerChildren.First();
for (uint32_t i = 1; i < index; i++)
insertAfter.MoveNext();
containerChildren.InsertAbove(compVisual, insertAfter.Current());
}

void Remove(const winrt::Microsoft::ReactNative::Composition::Experimental::IVisual &visual) noexcept {
auto compVisual = TTypeRedirects::CompositionContextHelper::InnerVisual(visual);
auto containerChildren = InnerVisual().as<typename TTypeRedirects::ContainerVisual>().Children();
containerChildren.Remove(compVisual);
auto it = std::find_if(m_childrenCache.begin(), m_childrenCache.end(), [&visual](const auto &cached) {
return cached == visual;
});
if (it != m_childrenCache.end()) {
m_childrenCache.erase(it);
}
}

winrt::Microsoft::ReactNative::Composition::Experimental::IVisual GetAt(uint32_t index) noexcept {
auto containerChildren = m_visual.as<typename TTypeRedirects::ContainerVisual>().Children();
auto it = containerChildren.First();
for (uint32_t i = 0; i < index; i++)
it.MoveNext();
return TTypeRedirects::CompositionContextHelper::CreateVisual(it.Current());
return m_childrenCache[index];
}
Comment on lines 464 to 466
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 18, 2026

Choose a reason for hiding this comment

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

P2 Unchecked vector index in noexcept function

operator[] is undefined behaviour when index >= m_childrenCache.size(). Because the method is marked noexcept you can't use .at() here (that would call std::terminate on OOB), so the right fix is an explicit guard. The original WinRT-iterator path would also crash on an invalid index, so the risk level is unchanged, but since this PR is already touching this method it's a good moment to add an assert:

  winrt::Microsoft::ReactNative::Composition::Experimental::IVisual GetAt(uint32_t index) noexcept {
    assert(index < m_childrenCache.size());
    return m_childrenCache[index];
  }


void SetClippingPath(ID2D1Geometry *clippingPath) noexcept {
Expand Down Expand Up @@ -534,6 +542,7 @@ struct CompVisualImpl {

protected:
TVisual m_visual;
std::vector<winrt::Microsoft::ReactNative::Composition::Experimental::IVisual> m_childrenCache;
};

template <typename TTypeRedirects>
Expand Down Expand Up @@ -1255,6 +1264,12 @@ struct CompScrollerVisual : winrt::implements<
std::sort(snapPositions.begin(), snapPositions.end());
snapPositions.erase(std::unique(snapPositions.begin(), snapPositions.end()), snapPositions.end());

// Skip reconfiguration if snap points haven't changed
if (snapPositions == m_previousSnapPositions) {
return;
}
m_previousSnapPositions = snapPositions;

std::vector<typename TTypeRedirects::InteractionTrackerInertiaRestingValue> restingValues;

for (size_t i = 0; i < snapPositions.size(); ++i) {
Expand Down Expand Up @@ -1384,6 +1399,7 @@ struct CompScrollerVisual : winrt::implements<
winrt::Microsoft::ReactNative::Composition::Experimental::SnapPointsAlignment m_snapToAlignment{
winrt::Microsoft::ReactNative::Composition::Experimental::SnapPointsAlignment::Near};
std::vector<float> m_snapToOffsets;
std::vector<float> m_previousSnapPositions;
bool m_inertia{false};
bool m_custom{false};
bool m_interacting{false};
Expand Down
29 changes: 20 additions & 9 deletions vnext/Microsoft.ReactNative/Fabric/Composition/CompositionViewComponentView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include "CompositionViewComponentView.h"

#include <vector>

#include <AutoDraw.h>
#include <Fabric/AbiState.h>
#include <Fabric/AbiViewProps.h>
Expand Down Expand Up @@ -1013,15 +1015,24 @@ bool ComponentView::anyHitTestHelper(
facebook::react::Tag &targetTag,
facebook::react::Point &ptContent,
facebook::react::Point &localPt) const noexcept {
if (auto index = m_children.Size()) {
do {
index--;
targetTag = winrt::get_self<winrt::Microsoft::ReactNative::implementation::ComponentView>(m_children.GetAt(index))
->hitTest(ptContent, localPt);
if (targetTag != -1) {
return true;
}
} while (index != 0);
auto size = m_children.Size();
if (size == 0) {
return false;
}

// Collect children into a local vector to avoid repeated O(n) IVector::GetAt calls
std::vector<winrt::Microsoft::ReactNative::ComponentView> children;
children.reserve(size);
for (auto const &child : m_children) {
children.push_back(child);
}
Comment on lines +1023 to +1028
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 18, 2026

Choose a reason for hiding this comment

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

P2 Misleading complexity comment and potential allocation overhead

The comment says "repeated O(n) IVector::GetAt calls", implying the original loop was O(n²). In practice, WinRT IVector<T>::GetAt on a vector-backed collection (the common case) is O(1), so the original loop was already O(n) total. The new code introduces a per-call heap allocation and n COM AddRef/Release operations on a hot hit-testing path. For shallow trees (the common case in React Native views) this could be slower. Consider profiling to confirm this is a net win before committing to the extra overhead.


for (auto it = children.rbegin(); it != children.rend(); ++it) {
targetTag = winrt::get_self<winrt::Microsoft::ReactNative::implementation::ComponentView>(*it)
->hitTest(ptContent, localPt);
if (targetTag != -1) {
return true;
}
}

return false;
Expand Down