Skip to content

Improve efficiency of tag population#37228

Merged
peppy merged 4 commits intoppy:masterfrom
peppy:tag-population-improvement
Apr 7, 2026
Merged

Improve efficiency of tag population#37228
peppy merged 4 commits intoppy:masterfrom
peppy:tag-population-improvement

Conversation

@peppy
Copy link
Copy Markdown
Member

@peppy peppy commented Apr 7, 2026

  • No longer holds realm write transaction open while performing sqlite lookups.
  • No longer attempts a write transaction when it will be a noop.

I'll admit that this is maybe working around the actual realm write part being slow, but as I can't profile the issue locally, the sluggishness may actually be in sqlite for those users affected (since it's only been reported for tag population and not difficulty calculation?).

Regardless, this should fix the issue this iteration.

I also adjusted the user messaging to let them know why tag population is happening, since we've had some questions as to why it's running in the first place (it only happens once a month, so that's understandable).

Note that #36128 also exists and has valid improvements which can be addressed separately. This is intended to be something we can act on immediately.

…pletion

This was used in one place, but I foresee this being a more common
scenario. This also fixes an edge case where the dismiss process would
fail if completion happened on an async thread before the
`NotificationOverlay`'s scheduler could handle the initial ingress.
@peppy peppy added the area:database Deals with local realm database label Apr 7, 2026
- No longer holds realm write transaction open while performing sqlite
lookups.
- No longer attempts a write transaction when it will be a noop.

I'll admit that this is maybe working around the actual realm write part
being slow, but as I can't profile the issue locally, the sluggishness
may actually be in sqlite for those users affected (since it's only been
reported for tag population and not difficulty calculation?).

Regardless, this should fix the issue this iteration.
@peppy peppy force-pushed the tag-population-improvement branch from 5f4dbfa to 89f2c84 Compare April 7, 2026 08:44
@cyperdark
Copy link
Copy Markdown

hello i have this PR #36128 which have larger scope but improves tags population and other BackgroundDataStoreProcessor processes as well

but currently it lacks benchmark project and unfortunately i have not much knowledge of C# nor of codebase to make it, so as was suggested im waiting on someone from osu dev team to make it instead: #36128 (comment)

@peppy
Copy link
Copy Markdown
Member Author

peppy commented Apr 7, 2026

See my note in the OP. Your effort is orthogonal to what I'm doing here. This change doesn't require benchmarking or much review attention because it's doing very obvious things that we're used to.

@cyperdark
Copy link
Copy Markdown

oh shit i might've had old page open and didnt see note, sorry

@peppy
Copy link
Copy Markdown
Member Author

peppy commented Apr 7, 2026

@cyperdark Once this is merged, if you have the energy please rebase your PR. Also consider splitting it out into multiple pieces. For instance, the reusable sqlite connection should be its own PR to make reviewing and merging easier. I'm more inclined to consider that one before the batching one (because as we've seen, the batch commits still showed overheads in your testing).

@bdach bdach self-requested a review April 7, 2026 10:49
@bdach bdach force-pushed the tag-population-improvement branch from 9425670 to 3197e5b Compare April 7, 2026 12:23
@pull-request-size pull-request-size bot added size/M and removed size/L labels Apr 7, 2026
Copy link
Copy Markdown
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

even in a naive one-shot my-machine manual stopwatch check against master, this is an order of magnitude faster on a large database I test with than master. so we can probably start here and see whether any of the remaining tweaks are even really necessary for most.

@peppy please double-check that I resolved the conflicts with master in the way you would expect. squash merges are not really that compatible with PR chains.

@peppy peppy merged commit 84cce2f into ppy:master Apr 7, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:database Deals with local realm database size/M

Projects

None yet

3 participants