[seyedmostafahasani]

Summary:

ViewabilityHelper._onUpdateSync filtered viewableIndicesToCheck with this._viewableIndices.includes(ii), which is O(n) per index and therefore O(n^2) per update. The filter is dead code: onUpdate assigns the array to this._viewableIndices right before calling _onUpdateSync, and the minimumViewTime timer already returns early when _updateGeneration has changed (every reassignment of _viewableIndices bumps it). Both call sites therefore pass the very same array that is stored in _viewableIndices, so the filter always keeps every index.

This removes the filter and uses viewableIndicesToCheck directly. The PR is a 4-line deletion with no behavior change.

Changelog:

[INTERNAL] [REMOVED] - Remove no-op stale-index filter in ViewabilityHelper

Test Plan:

No behavior change. The existing ViewabilityHelper-test.js cases cover this path, in particular "minimumViewTime ignores stale viewability updates". Run with yarn jest packages/virtualized-lists/Lists.

Performance:

The main benefit is removing dead code. Measured on the real ViewabilityHelper.onUpdate (Node v22 + Jest, 20k-item list, scrolling one item per tick so the viewable set changes on every update, 300 updates per run, median of 7 interleaved runs) with an earlier iteration of this PR that skipped the filter via an identity check. That variant does the same work as removing the filter for these call sites.

Viewable items	main (µs/update)	Filter skipped (µs/update)
10	152	157 (no difference)
50	376	380 (no difference)
200	1,182	1,111 (~6% faster, within noise)
800	4,623	4,303 (~7% faster)

At typical sizes (~10 viewable items) the change is neutral. Run-to-run ranges overlap, so only the 800-item gain is plausibly real.

[meta-cla]

Hi @seyedmostafahasani!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[javache]

_viewableIndices is often on the order of magnitude of 10, not 50, so your benchmarks show this wouldn't be beneficial. Perhaps if we computed this set inside computeViewableItems? Even then, it's probably a negligible win - data locality in arrays outperforms algorithmic complexity for small numbers.

[seyedmostafahasani]

Thanks for the review, and agreed on the numbers. In my benchmark the Set only breaks even at around 50-100 viewable indices (122 ns vs 363 ns at n=10, so ~3x slower there), and with _viewableIndices typically around 10 this change as written wouldn't help most lists.

I'll try your suggestion of building the Set once in computeViewableItems and measure end-to-end (a scroll benchmark in RNTester with small and large viewable windows) rather than just the isolated filter. If it doesn't show a clear win at typical sizes, I'll close this PR.

[seyedmostafahasani]

Thanks again for the review. I tried your suggestion and also looked at the call sites more closely.

Measured on the real ViewabilityHelper.onUpdate (scroll simulation over a 20k-item list, render range like VirtualizedList, median of 7 runs): building the Set in onUpdate instead of _onUpdateSync makes no difference, since it is built once per changed update either way. At ~10 viewable items none of the variants is distinguishable from the baseline (~16 us per update, within the +/-5-10% noise), so I agree it is negligible at typical sizes.

While reading the code I noticed the filter in _onUpdateSync looks like a no-op. Both call sites pass the same array that was just stored in this._viewableIndices, and the minimumViewTime timer already returns early when _updateGeneration changed (every reassignment of _viewableIndices bumps it). So the filter always keeps every index.

Instead of the Set, I'd change this PR to skip the filter when the input is the stored array:

if (viewableIndicesToCheck !== this._viewableIndices) {
  viewableIndicesToCheck = viewableIndicesToCheck.filter(ii =>
    this._viewableIndices.includes(ii),
  );
}

Behavior stays identical for any other caller, and all 22 existing ViewabilityHelper tests pass. Measured speedup vs. current main: ~1.0-1.1x at 10-50 viewable items (within noise), ~1.2x at ~200, ~1.6x at ~800. So the win only matters for very large viewable sets.

If you'd prefer removing the dead filter entirely, or think this isn't worth changing, I'm happy to do that or close the PR instead.

[javache]

Please make the change in the PR - would be great if you could have data on time spent in JS per scroll before/after

[seyedmostafahasani]

Updated the PR as discussed: _onUpdateSync now skips the filter when viewableIndicesToCheck === this._viewableIndices (the case for both call sites) and keeps it for any other caller. The Set is gone, so the net diff is just the identity guard.

JS time in ViewabilityHelper.onUpdate per scroll update, before/after

Setup: real ViewabilityHelper.onUpdate under Node v22 + Jest, 20k-item list, scrolling one item per tick so the viewable set changes on every update, 300 updates per run, median of 7 interleaved runs. Results (changed items) are identical across all three variants.

Viewable items	main (µs/update)	Set version	Identity guard	Guard vs main
10	152	157	157	no difference
50	376	384	380	no difference
200	1,182	1,102	1,111	~6% faster (within noise)
800	4,623	4,490	4,303	~7% faster

Run-to-run ranges overlap (e.g. 800 items: 1,324–1,555 ms main vs 1,137–1,425 ms guard per 300 updates), so only the 800-item gain is plausibly real. At typical sizes (~10 viewable items) the change is neutral, so it's your call whether it's worth landing.

[javache]

There's already an early bail-out here: https://github.com/react/react-native/blob/main/packages/virtualized-lists/Lists/ViewabilityHelper.js#L236

_onUpdateSync is called after assigning it here https://github.com/react/react-native/blob/main/packages/virtualized-lists/Lists/ViewabilityHelper.js#L245C5-L245C44 - so this filter will always skip - can we just drop it completely?

[seyedmostafahasani]

@javache thanks, makes sense. I dropped the filter completely in 7e38da8, so _onUpdateSync now just uses viewableIndicesToCheck directly. The PR is now only a 4-line deletion with no behavior change. Could you take another look?

[meta-codesync]

@javache has imported this pull request. If you are a Meta employee, you can view this in D128673216.

[javache]

Can you update the PR title and description to reflect the change made?

[seyedmostafahasani]

@javache I've updated the PR title and description to reflect the final change (dropping the dead stale-index filter). Could you take another look?