GXS: fix four causes of slow channel message loading - #351
Open
jolavillette wants to merge 8 commits into
Open
Conversation
jolavillette
force-pushed
the
perf/gxs-channel-loading
branch
from
July 27, 2026 20:06
84fd472 to
8b05f98
Compare
jolavillette
force-pushed
the
perf/gxs-channel-loading
branch
from
July 31, 2026 20:11
8b05f98 to
734dd18
Compare
csoler
requested changes
Jul 31, 2026
csoler
left a comment
csoler
left a comment
Contributor
There was a problem hiding this comment.
Could you plz remove all the extra profiling code? Of course you may keep the profiling classes themselves. This will make the PR much easier to read.
Contributor
Author
|
done |
jolavillette
force-pushed
the
perf/gxs-channel-loading
branch
4 times, most recently
from
August 4, 2026 18:29
6c4dd0f to
cc520e5
Compare
Loading a channel with a few thousand posts takes ten seconds or more, and the cost is spread over four layers with no way to tell which one dominates: SQL retrieval in RsDataService, deserialisation in RsGenExchange, conversion to service structures in p3GxsChannels, then the model update in the GUI. Add a small header-only helper (gxs/rsgxsprofiler.h) and instrument those layers so each reports its own breakdown on one line. Profiling stays off unless the RS_GXS_PROFILE environment variable is set; its value is a reporting threshold in milliseconds so only the operations worth looking at show up (RS_GXS_PROFILE=0 reports everything). The reported counters are the ones that matter for the known bottlenecks: number of SQL queries issued and blob volume read in retrieveNxsMsgs, number of metas walked in retrieveGxsMsgMetaData, mGenMtx wait and deserialisation time in getMsgData, and the token wait in getChannelAllContent. No behaviour change: when profiling is disabled the added work is a couple of steady_clock reads per call and one comparison against a cached threshold. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
RsGxsDataAccess::getMsgData() always ran the request through getMsgIdList(), which walks every message meta of the group and returns the explicit list of matching message ids. That list was then handed to RsDataService, whose retrieveNxsMsgs() has two paths: a single "WHERE grpId=..." query when the id set is empty, and one prepared query per message otherwise. Since a request for a whole group carries an empty id set precisely to mean "all messages", expanding it into 6400 explicit ids meant the fast path was never taken when opening a channel: the store issued 6400 separate sqlite3_prepare_v2 + step + finalize cycles against an encrypted database, plus a full preliminary pass over the metas that produced nothing the caller did not already know. When none of mStatusMask, mMsgFlagMask, MSG_LATEST, MSG_ORIGMSG or MSG_THREAD is set, no filtering can occur, so pass the request through untouched. The resulting message set is identical, the meta pass disappears, and loading a whole group collapses to a single SQL query. Requests that do filter are unaffected and keep the previous path. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…message RsDataService::retrieveNxsMsgs() issued one prepared statement per requested message id. Each one rebuilds the SQL text, runs a full sqlite3_prepare_v2 (SQL parse plus query planner), allocates a cursor and finalizes it -- an overhead that dominates the actual row lookup, and is paid thousands of times whenever a request covers a large id set. Pack the ids into "msgId IN (...)" batches of 500 instead. The message ids are plain hex strings so they need no escaping, and the batch size keeps both the generated SQL and sqlite's expression tree small. The previous commit removed this path for unfiltered whole-group requests; this one covers everything else: a channel post with its comments, forum threads, and any filtered request. retrieveGxsMsgMetaData() still has the same one-query-per-id shape in its non-empty branch, but there each id is first looked up in the meta cache, so the remaining queries are only the cache misses. Left alone for now so it can be measured on its own. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
RsGxsImage owns a malloc'd buffer and declares a destructor, a copy constructor and a copy assignment. RsMsgMetaData and RsGxsGenericMsgData declare destructors too, and so does RsGxsChannelPost. Each of those user declared destructors suppresses the implicit move operations, so every std::move() on a post silently resolved to the copy constructor: a malloc plus memcpy of the thumbnail. That cost is paid far more often than it looks. A channel's post array is copied whole at four hand-off points between the store, the service and the model, none of which reserve, so vector growth copies on top. Worse, std::sort falls back to copies as well, which for a few thousand posts means tens of thousands of thumbnail duplications -- and that sort runs in the GUI thread. Give RsGxsImage real move operations that steal the buffer, and explicitly default the copy and move operations of RsMsgMetaData, RsGxsGenericMsgData and RsGxsChannelPost. The resulting RsGxsChannelPost move constructor is noexcept, which is what std::vector requires to move rather than copy on reallocation. Then use them where the arrays are handed over: reserve and move in getPostData() and sortPosts() instead of copying element by element, and move the sorted array back into the caller's vector. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
A channel keeps every version of every edited post. Only the latest of each
chain is ever shown: sortPosts() read them all, then discarded the superseded
ones keeping just their ids in mOlderVersions. Profiling a real channel shows
how much that costs: 6409 messages read for 1931 displayed posts, 195MB of
payload of which roughly two thirds belongs to versions thrown away
immediately -- thumbnails decrypted, deserialised and freed for nothing.
Resolve the version chains on the metas instead. They are small, come from
the meta cache once warm, and sortPostMetas() already works on any type
exposing a RsMsgMetaData. getChannelAllContent() now:
- pulls the group's metas via getContentSummaries()
- splits posts from comments and votes
- runs sortPostMetas() to find the retained version of each chain
- requests message data for those ids only, plus all comments and votes
Since the request now carries an explicit id set, it goes through the batched
IN(...) retrieval added earlier: a few queries instead of one, and the payload
read drops by whatever the edit history weighs.
Two behaviours of sortPosts() have to be reproduced, and applyPostVersions()
does so from the resolved chains:
- comments hang off whichever version was current when they were written, so
they are remapped onto the retained post before being counted, which
replaces the old "add up the counts of all older versions" pass;
- sortPostMetas() normalises mOrigMsgId to the top of the chain, and callers
match edited posts on that value (GUI updateSinglePost), so the normalised
id is carried over to the post that is returned.
The item conversion loop is factored out of getPostData() into
convertMsgItems() so both paths share it; getPostData() itself, still used by
getChannelContent() and the deprecated API, keeps calling sortPosts()
unchanged.
An empty id set means "every message of the group" to the data store, so an
empty channel returns before any request is made rather than asking for
everything.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Profiling the version filtering added in the previous commit showed the batched IN(...) retrieval costing a near constant 60 to 95ms per batch whatever the number of messages it returned: 5 batches for 2203 messages took 454ms, 7 batches for 3127 took 451ms. That is the signature of each batch walking the whole group, and it made a channel with almost no edited posts slower than before (272ms in one query against 451ms in seven). The cause is the query plan. On "grpId=... AND msgId IN (...)" sqlite has no ANALYZE data, so it estimates an equality on the non unique group index at about ten rows, against one row per entry of the IN list. The group therefore looks fifty times more selective than it really is -- it matches every message of the channel -- and gets picked, so every batch scans the group and filters. Select on the message id alone. It is the table's primary key, hence unique table wide, so the result is identical while sqlite can seek straight into the implicit unique index. locked_retrieveMessages() takes an optional expected group and drops anything else, so a caller mixing groups still cannot get foreign messages attributed to the wrong one. Reading most of a group by id remains slower than scanning it once, whatever the index. So getChannelAllContent() now only filters versions out when they are worth filtering: past MAX_READ_RATIO_FOR_VERSION_FILTERING of the group's messages it requests the whole group and resolves versions with sortPosts() as before. The profiling line reports which path was taken. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Requested in review: the instrumentation added by the first commit served to measure the four defects and verify the fixes, but it should not stay in the optimised code. The probes are removed from rsdataservice, rsgenexchange and p3gxschannels; convertMsgItems() loses the two timing out-parameters that only existed to feed them. The profiler class itself (gxs/rsgxsprofiler.h) is kept, per review, for future measurement work. Nothing includes it anymore. Co-Authored-By: Claude Fable 5 <[email protected]>
jolavillette
force-pushed
the
perf/gxs-channel-loading
branch
from
August 6, 2026 23:17
cc520e5 to
494ce38
Compare
csoler
requested changes
Aug 8, 2026
| std::set<RsGxsMessageId> wanted_msgs; | ||
|
|
||
| for(auto& m: metas) | ||
| if(m.mThreadId.isNull() && m.mParentId.isNull()) |
Contributor
There was a problem hiding this comment.
For a top level comment:
- mThreadId is equal to the actual post the comment refers to, so it's never null,
- mParentId can be actually null if following the old comment paradigm (in the code now, it's never null)
For votes, - mThreadId is the post id which comment is voted,
- mParentId is the comment that is voted.
For posts,
- mParentId is always null (see p3gxschannels.cc:2021)
- mThreadId is always null (see p3gxschannels.cc:2015)
So the test above should be safer to only look for mThreadId.isNull() in order to accurately discard what is not a post.
Contributor
Author
There was a problem hiding this comment.
Right, thanks — fixed in 2a2a1e2.
mParentId proves nothing here since old-paradigm comments have a null one, so the test is now on mThreadId alone, with a comment recording the taxonomy you described.
For the record, the composite test comes from getChannelStatistics() (p3gxschannels.cc:1721), which I used as a model, and the same one is in p3wire.cc:1837 for pulses. I left both untouched as they are out of this PR's scope — happy to align them here or in a separate PR, your call.
getChannelAllContent() split the metas into posts and comments/votes on "mThreadId.isNull() && mParentId.isNull()". Only the first half of that test carries information: a post never has a thread id, a comment always carries the id of the post it belongs to, and a vote the id of the post whose comment is being voted. mParentId proves nothing, since comments written under the old comment paradigm have a null one. Reported by csoler on PR RetroShare#351. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GXS: fix four causes of slow channel message loading
works with retroshare-gui pr/3280
RetroShare/RetroShare#3280
Opening a channel with 6409 messages took several seconds. It now takes about half a second on the same profile. Four independent defects, each measured before and after.
One SQL query per message. A request for a whole group carries an empty message id set, meaning "everything". RsGxsDataAccess::getMsgData() expanded it into the explicit list of all 6409 ids before reaching the store, which forced the per-message query path: 6409 sqlite3_prepare_v2 cycles against an encrypted database. The single-query path existed and was never taken.
Superseded post versions were read in full. A channel keeps every version of every edited post; only the latest is displayed. sortPosts() read them all and discarded the old ones after decrypting and deserialising them — 195 MB read to display 23 MB worth of posts. Version chains are now resolved on the metas, which are small and cached, and only the retained versions are read.
No move semantics. RsGxsImage, RsMsgMetaData, RsGxsGenericMsgData and RsGxsChannelPost all declare destructors, which suppresses the implicit move operations, so every std::move() silently became a malloc+memcpy of the thumbnail. The post array was deep copied at four hand-off points.
A query planner trap. With no ANALYZE data sqlite estimates an equality on a non-unique index at ~10 rows, so grpId=? AND msgId IN (...) picked the group index and rescanned the whole group once per batch. Selecting on the message id alone — it is the primary key, unique table wide — fixes it.
No schema change, no public API change, no behaviour change: returned posts, comments, votes and comment counts are identical on every path.
The first commit adds opt-in profiling (RS_GXS_PROFILE=0), which is what makes all of the above verifiable. Happy to drop it if you would rather not carry it.
Review focus: applyPostVersions() reproduces two behaviours of sortPosts() — comments hanging off a superseded version must still be counted on the retained post, and the mOrigMsgId normalisation that updateSinglePost() matches on must be carried over.
Companion PR in the GUI repository: pr/3280. It needs this one merged first.