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 song select realm refresh performance #25838

Merged
merged 14 commits into from
Dec 19, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Dec 18, 2023

Closes #25824

Each commit is basically standalone.

If any are seen as hard-to-merge or break test expectations, we can cherry-pick the good ones for now.

87b7699 takes things from <1 FPS to a substantially usable frame rate with occasional stutters.

The rest of the commits work to reduce the stutter down to <100ms.

Can be tested using test database

@peppy peppy requested a review from bdach December 18, 2023 11:51
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Dec 18, 2023
@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

The invalidation logic changes are very difficult to follow. I'm still not sure whether behaviourally BeatmapSetsLoaded is always in the correct state now, or if BeatmapSetsChanged is invoked when it should be and not invoked when it shouldn't.

The test failures are not encouraging. Not sure if you want to look into them, or should I.

@peppy
Copy link
Member Author

peppy commented Dec 18, 2023

The invalidation logic changes are very difficult to follow. I'm still not sure whether behaviourally BeatmapSetsLoaded is always in the correct state now, or if BeatmapSetsChanged is invoked when it should be and not invoked when it shouldn't.

The test failures are not encouraging. Not sure if you want to look into them, or should I.

Originally as far as I can tell it was only guarding in the realm callback, when the changeset is null. This is guaranteed by realm to only occur once, on initial event subscription.

@peppy
Copy link
Member Author

peppy commented Dec 18, 2023

I've reverted one change for now in the interest of getting this in. Will revisit it as a separate PR.

@peppy
Copy link
Member Author

peppy commented Dec 18, 2023

One test remains flaky, trying to figure out what's going on but have not repro'd locally yet.

I think this is required because there is a higher chance of batched
updates with the new structure (and less calls to `BeatmapSetsChanged`
which causes re-selection).
@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

Originally as far as I can tell it was only guarding in the realm callback, when the changeset is null. This is guaranteed by realm to only occur once, on initial event subscription.

No, null changeset will also happen after a realm recycle. Compare: #25821.

I'm not sure what that means for this diff, since the rest of your answer is rather enigmatic, but there.

Edit: It appears irrelevant now that I look at it again. With the latest changes I feel much more confident that BeatmapCarousel behaviour is unchanged (within the scope of the value of BeatmapsLoaded and firing of BeatmapsChanged).

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 18, 2023
@bdach
Copy link
Collaborator

bdach commented Dec 19, 2023

Tests are failing because after beatmap removal the difficulty count under search bar does not update properly anymore. It shows stale counts from before the deletion.

Previously the deletion of items from carousel would happen before summing up the difficulty counts, but now it happens after. So this is a regression from 8f5d21d specifically.

@peppy
Copy link
Member Author

peppy commented Dec 19, 2023

Easy fix, luckily. Just recalculate after deletion (should have been there all along).

If another test breaks I will not be a happy peppy.

@bdach
Copy link
Collaborator

bdach commented Dec 19, 2023

Appears to be fine now, but I want this to go through a CI run with master merged before merging just to make sure I don't get clowned by unexpected crosstalk between this and the filtering fix.

@bdach bdach merged commit b869973 into ppy:master Dec 19, 2023
17 checks passed
@peppy peppy deleted the fix-song-select-realm-refresh-performance branch December 19, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:song-select next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/L type:performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Song Select keeps returning to currently selected map after releasing right click
2 participants