Repository navigation
Fix ClickHouse queue stalling from per-entity UUID lookups - #1038
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e414022ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| firstRemaining++; | ||
| } | ||
| if (firstRemaining < entitySpawnUpdateData.size()) { | ||
| entitySpawnUpdates.prefetch(new ArrayList<>(entitySpawnUpdateData.subList(firstRemaining, entitySpawnUpdateData.size()))); |
There was a problem hiding this comment.
When a ClickHouse buffer spans multiple commits, entitySpawnUpdateData now remains full for the entire method so it can be reused here; copying a suffix does not release the consumed prefix. releaseProcessedConsumerData nulls the committed queue/map entries after each successful batch, but this local list still strongly references every processed EntitySpawnData (including location/state arrays), and the parallel boxed-index list is never trimmed either. On the multi-million-update backlogs this change targets, those payloads therefore cannot be reclaimed and can exhaust the heap; trim both consumed prefixes or retain only a bounded unprocessed window after each prefetch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, idk if something like an inner class that stores the prefetch data and indexes would be a better choice here
|
Thanks for tracking this down. Restoring bulk prefetching after batch boundaries makes sense. Please trim the consumed prefixes from both lists after each successful commit, keeping their remaining entries aligned and preserving the original queue indices. Otherwise, the original list keeps committed entity payloads alive after the queue releases them. Once trimmed, the remaining data list can be passed directly to prefetch() without creating another copy. That should be enough for this fix. |
|
Done! |
2 participants
Along with the memory leak issue I PRed earlier, I also noticed CoreProtect's database consumer queue stalling during the day and especially peak hours (with over 350 concurrent players). Some thread dumps later I finally traced it to the ClickHouse consumer spending nearly all its time on repeated entity UUID lookups.
Entity spawn updates (mostly location updates) first prefetch the existing rows for all their UUIDs in a few large queries. That cache is cleared every time a batch is committed, which happens about every 100k rows. After the first commit, the rest of the updates in the same run had nothing cached, so each one fell back to its own query for a single UUID. Each of those queries uses FINAL, which has to merge across all parts and read around 300k rows no matter how few UUIDs are asked for. At our scale that came to 150–270 ms each, or roughly 5 lookups per second. The consumer spent around 98% of its time waiting on them, and the queue grew past 15M entries in a day. It took until 10AM the next day for the queue to fully clear due to the sheer amount of data.
I've changed the processor to keep the entity updates for the whole run along with their position in the consumer data. After each mid-run commit the remaining updates are prefetched again, so lookups come from the cache instead of the database. The block target lookups for entity rollbacks go through the same prefetch, so they benefit too. After the change, time spent on SELECTs dropped from about 59 s to about 6 s per minute, and inserts went from bursts of 0–20 per minute to a steady ~55.