Skip to content
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.

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

Fix search container filtering too much #5578

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Dec 11, 2022

As it turns out, 99915f9 introduced a performance regression in SearchContainer. Due to borrowing from #5154 too much (see relevant discord conversation), the search logic was changed in order to hook into InvalidateLayout() to perform search, rather than just do it when a new internal child was added, as was the case previously.

For the sake of completeness, let's review the possible causes for InvalidateLayout() firing with respect to filtering:

  • AddInternal(): Valid, as the new child needs to be examined against the current filter, and preserved by reverting to the old code.
  • RemoveInternal(): Mostly irrelevant, as long as the presence of the removed drawable in the container doesn't impact the filter state of any of its siblings. Let's not support that case for now.
  • ClearInternal(): Totally irrelevant, as anything that could be filtered is now gone.
  • SetLayoutPosition(): Hopefully irrelevant.
  • UpdateChildrenLife(): Not relevant. Nothing in SearchContainer considers lifetime explicitly.
  • FlowContainer.childLayout becoming invalid: The invalidation flags there are RequiredParentSizeToFit | Presence. Neither should be relevant for filtering, given the direction taken with IConditionalFilterable.
  • Direction and Spacing: 100% irrelevant.

@peppy Would appreciate it if you ran this through your perf wringer and see if it helps any with the blocking load issue pointed out on discord. This does nothing for the JIT overhead pointed out therein, but this pull should cull some hundreds of extraneous filters that could be triggered on some (but not all, from my testing) game-side settings sidebar loads, and those extra calls mostly stemmed from the childLayout invalidations - so I before I dig in any further I want to see if this at least restores baseline performance.

As it turns out, 99915f9 introduced a
performance regression in `SearchContainer`. Due to borrowing from ppy#5154
too much, the search logic was changed in order to hook into
`InvalidateLayout()` to perform search, rather than just do it when a
new internal child was added, as was the case previously.

For the sake of completeness, let's review the possible causes for
`InvalidateLayout()` firing with respect to filtering:

- `AddInternal()`: Valid, as the new child needs to be examined against
  the current filter, and preserved by reverting to the old code.
- `RemoveInternal()`: Mostly irrelevant, as long as the presence of the
  removed drawable in the container doesn't impact the filter state of
  any of its siblings. Let's not support that case for now.
- `ClearInternal()`: Totally irrelevant, as anything that could be
  filtered is now gone.
- `SetLayoutPosition()`: Hopefully irrelevant.
- `UpdateChildrenLife()`: Not relevant. Nothing in `SearchContainer`
  considers lifetime explicitly.
- `FlowContainer.childLayout` becoming invalid: The invalidation flags
  there are `RequiredParentSizeToFit | Presence`. Neither should be
  relevant for filtering, given the direction taken with
  `IConditionalFilterable`.
- `Direction` and `Spacing`: 100% irrelevant.
@peppy
Copy link
Member

peppy commented Dec 12, 2022

Change seems good, and may help more in other cases, but doesn't seem to do much here (as expected, the JIT is still the killer):

Before:

4,444 calls to matchSubTree()

JIT: 104ms
User: 22ms

After:

2,229 calls to matchSubTree()

JIT: 104ms
User: 20ms

@peppy peppy enabled auto-merge December 12, 2022 08:05
@peppy peppy merged commit a938822 into ppy:master Dec 12, 2022
@bdach bdach deleted the search-container-perf-regression branch December 12, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants