From 13dc3065063317d4dc28a33bc8d300511064c283 Mon Sep 17 00:00:00 2001 From: iProdigy <8106344+iProdigy@users.noreply.github.com> Date: Sat, 9 Dec 2023 10:46:30 -0800 Subject: [PATCH] perf: query fewer historical messages on reconnects (#5001) Co-authored-by: Ruben Anders Co-authored-by: pajlada --- CHANGELOG.md | 1 + src/common/Channel.hpp | 2 - src/providers/irc/AbstractIrcServer.cpp | 8 +- src/providers/recentmessages/Api.cpp | 124 +++++++++++---------- src/providers/recentmessages/Api.hpp | 14 ++- src/providers/recentmessages/Impl.cpp | 25 ++++- src/providers/recentmessages/Impl.hpp | 7 +- src/providers/twitch/IrcMessageHandler.cpp | 1 + src/providers/twitch/TwitchChannel.cpp | 53 +++++++-- src/providers/twitch/TwitchChannel.hpp | 13 +++ 10 files changed, 171 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bc065e7..e13735bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,7 @@ - Dev: Move `clang-tidy` checker to its own CI job. (#4996) - Dev: Refactored the Image Uploader feature. (#4971) - Dev: Fixed deadlock and use-after-free in tests. (#4981) +- Dev: Load less message history upon reconnects. (#5001) ## 2.4.6 diff --git a/src/common/Channel.hpp b/src/common/Channel.hpp index 4fc686c0..de134b12 100644 --- a/src/common/Channel.hpp +++ b/src/common/Channel.hpp @@ -61,8 +61,6 @@ public: pajlada::Signals::Signal &> filledInMessages; pajlada::Signals::NoArgSignal destroyed; pajlada::Signals::NoArgSignal displayNameChanged; - /// Invoked when AbstractIrcServer::onReadConnected occurs - pajlada::Signals::NoArgSignal connected; Type getType() const; const QString &getName() const; diff --git a/src/providers/irc/AbstractIrcServer.cpp b/src/providers/irc/AbstractIrcServer.cpp index 58fda00f..9c30049e 100644 --- a/src/providers/irc/AbstractIrcServer.cpp +++ b/src/providers/irc/AbstractIrcServer.cpp @@ -5,6 +5,7 @@ #include "messages/LimitedQueueSnapshot.hpp" #include "messages/Message.hpp" #include "messages/MessageBuilder.hpp" +#include "providers/twitch/TwitchChannel.hpp" #include @@ -331,8 +332,6 @@ void AbstractIrcServer::onReadConnected(IrcConnection *connection) { chan->addMessage(connectedMsg); } - - chan->connected.invoke(); } this->falloffCounter_ = 1; @@ -360,6 +359,11 @@ void AbstractIrcServer::onDisconnected() } chan->addMessage(disconnectedMsg); + + if (auto *channel = dynamic_cast(chan.get())) + { + channel->markDisconnectedNow(); + } } } diff --git a/src/providers/recentmessages/Api.cpp b/src/providers/recentmessages/Api.cpp index aad37234..efce6d33 100644 --- a/src/providers/recentmessages/Api.cpp +++ b/src/providers/recentmessages/Api.cpp @@ -18,72 +18,84 @@ namespace chatterino::recentmessages { using namespace recentmessages::detail; -void load(const QString &channelName, std::weak_ptr channelPtr, - ResultCallback onLoaded, ErrorCallback onError) +void load( + const QString &channelName, std::weak_ptr channelPtr, + ResultCallback onLoaded, ErrorCallback onError, const int limit, + const std::optional> + after, + const std::optional> + before, + const bool jitter) { qCDebug(LOG) << "Loading recent messages for" << channelName; - const auto url = constructRecentMessagesUrl(channelName); + const auto url = + constructRecentMessagesUrl(channelName, limit, after, before); - NetworkRequest(url) - .onSuccess([channelPtr, onLoaded](const auto &result) { - auto shared = channelPtr.lock(); - if (!shared) - { - return; - } - - qCDebug(LOG) << "Successfully loaded recent messages for" - << shared->getName(); - - auto root = result.parseJson(); - auto parsedMessages = parseRecentMessages(root); - - // build the Communi messages into chatterino messages - auto builtMessages = - buildRecentMessages(parsedMessages, shared.get()); - - postToThread([shared = std::move(shared), root = std::move(root), - messages = std::move(builtMessages), - onLoaded]() mutable { - // Notify user about a possible gap in logs if it returned some messages - // but isn't currently joined to a channel - const auto errorCode = root.value("error_code").toString(); - if (!errorCode.isEmpty()) + const long delayMs = jitter ? std::rand() % 100 : 0; + QTimer::singleShot(delayMs, [=] { + NetworkRequest(url) + .onSuccess([channelPtr, onLoaded](const auto &result) { + auto shared = channelPtr.lock(); + if (!shared) { - qCDebug(LOG) - << QString("Got error from API: error_code=%1, " - "channel=%2") - .arg(errorCode, shared->getName()); - if (errorCode == "channel_not_joined" && !messages.empty()) - { - shared->addMessage(makeSystemMessage( - "Message history service recovering, there may " - "be gaps in the message history.")); - } + return; } - onLoaded(messages); - }); - }) - .onError([channelPtr, onError](const NetworkResult &result) { - auto shared = channelPtr.lock(); - if (!shared) - { - return; - } + qCDebug(LOG) << "Successfully loaded recent messages for" + << shared->getName(); - qCDebug(LOG) << "Failed to load recent messages for" - << shared->getName(); + auto root = result.parseJson(); + auto parsedMessages = parseRecentMessages(root); - shared->addMessage(makeSystemMessage( - QStringLiteral( - "Message history service unavailable (Error: %1)") - .arg(result.formatError()))); + // build the Communi messages into chatterino messages + auto builtMessages = + buildRecentMessages(parsedMessages, shared.get()); - onError(); - }) - .execute(); + postToThread([shared = std::move(shared), + root = std::move(root), + messages = std::move(builtMessages), + onLoaded]() mutable { + // Notify user about a possible gap in logs if it returned some messages + // but isn't currently joined to a channel + const auto errorCode = root.value("error_code").toString(); + if (!errorCode.isEmpty()) + { + qCDebug(LOG) + << QString("Got error from API: error_code=%1, " + "channel=%2") + .arg(errorCode, shared->getName()); + if (errorCode == "channel_not_joined" && + !messages.empty()) + { + shared->addMessage(makeSystemMessage( + "Message history service recovering, there may " + "be gaps in the message history.")); + } + } + + onLoaded(messages); + }); + }) + .onError([channelPtr, onError](const NetworkResult &result) { + auto shared = channelPtr.lock(); + if (!shared) + { + return; + } + + qCDebug(LOG) << "Failed to load recent messages for" + << shared->getName(); + + shared->addMessage(makeSystemMessage( + QStringLiteral( + "Message history service unavailable (Error: %1)") + .arg(result.formatError()))); + + onError(); + }) + .execute(); + }); } } // namespace chatterino::recentmessages diff --git a/src/providers/recentmessages/Api.hpp b/src/providers/recentmessages/Api.hpp index 2a558010..57193be1 100644 --- a/src/providers/recentmessages/Api.hpp +++ b/src/providers/recentmessages/Api.hpp @@ -2,8 +2,10 @@ #include +#include #include #include +#include #include namespace chatterino { @@ -28,8 +30,16 @@ using ErrorCallback = std::function; * @param channelPtr Weak pointer to Channel to use to build messages * @param onLoaded Callback taking the built messages as a const std::vector & * @param onError Callback called when the network request fails + * @param limit Maximum number of messages to query + * @param after Only return messages that were received after this timestamp; ignored if `std::nullopt` + * @param before Only return messages that were received before this timestamp; ignored if `std::nullopt` + * @param jitter Whether to delay the request by a small random duration */ -void load(const QString &channelName, std::weak_ptr channelPtr, - ResultCallback onLoaded, ErrorCallback onError); +void load( + const QString &channelName, std::weak_ptr channelPtr, + ResultCallback onLoaded, ErrorCallback onError, int limit, + std::optional> after, + std::optional> before, + bool jitter); } // namespace chatterino::recentmessages diff --git a/src/providers/recentmessages/Impl.cpp b/src/providers/recentmessages/Impl.cpp index 36a4b4c9..2f784ef3 100644 --- a/src/providers/recentmessages/Impl.cpp +++ b/src/providers/recentmessages/Impl.cpp @@ -5,7 +5,6 @@ #include "providers/twitch/IrcMessageHandler.hpp" #include "providers/twitch/TwitchChannel.hpp" #include "providers/twitch/TwitchMessageBuilder.hpp" -#include "singletons/Settings.hpp" #include "util/FormatTime.hpp" #include @@ -94,14 +93,34 @@ std::vector buildRecentMessages( // Returns the URL to be used for querying the Recent Messages API for the // given channel. -QUrl constructRecentMessagesUrl(const QString &name) +QUrl constructRecentMessagesUrl( + const QString &name, const int limit, + const std::optional> + after, + const std::optional> + before) { QUrl url(Env::get().recentMessagesApiUrl.arg(name)); QUrlQuery urlQuery(url); if (!urlQuery.hasQueryItem("limit")) + { + urlQuery.addQueryItem("limit", QString::number(limit)); + } + if (after.has_value()) { urlQuery.addQueryItem( - "limit", QString::number(getSettings()->twitchMessageHistoryLimit)); + "after", QString::number( + std::chrono::duration_cast( + after->time_since_epoch()) + .count())); + } + if (before.has_value()) + { + urlQuery.addQueryItem( + "before", QString::number( + std::chrono::duration_cast( + before->time_since_epoch()) + .count())); } url.setQuery(urlQuery); return url; diff --git a/src/providers/recentmessages/Impl.hpp b/src/providers/recentmessages/Impl.hpp index 000d379c..3c60b6c3 100644 --- a/src/providers/recentmessages/Impl.hpp +++ b/src/providers/recentmessages/Impl.hpp @@ -8,7 +8,9 @@ #include #include +#include #include +#include #include namespace chatterino::recentmessages::detail { @@ -24,6 +26,9 @@ std::vector buildRecentMessages( // Returns the URL to be used for querying the Recent Messages API for the // given channel. -QUrl constructRecentMessagesUrl(const QString &name); +QUrl constructRecentMessagesUrl( + const QString &name, int limit, + std::optional> after, + std::optional> before); } // namespace chatterino::recentmessages::detail diff --git a/src/providers/twitch/IrcMessageHandler.cpp b/src/providers/twitch/IrcMessageHandler.cpp index 0bfef74a..183a483e 100644 --- a/src/providers/twitch/IrcMessageHandler.cpp +++ b/src/providers/twitch/IrcMessageHandler.cpp @@ -1136,6 +1136,7 @@ void IrcMessageHandler::handleJoinMessage(Communi::IrcMessage *message) getApp()->accounts->twitch.getCurrent()->getUserName()) { twitchChannel->addMessage(makeSystemMessage("joined channel")); + twitchChannel->joined.invoke(); } else if (getSettings()->showJoins.getValue()) { diff --git a/src/providers/twitch/TwitchChannel.cpp b/src/providers/twitch/TwitchChannel.cpp index 14670769..e2826fd4 100644 --- a/src/providers/twitch/TwitchChannel.cpp +++ b/src/providers/twitch/TwitchChannel.cpp @@ -103,17 +103,12 @@ TwitchChannel::TwitchChannel(const QString &name) // We can safely ignore this signal connection this has no external dependencies - once the signal // is destroyed, it will no longer be able to fire - std::ignore = this->connected.connect([this]() { - if (this->roomId().isEmpty()) + std::ignore = this->joined.connect([this]() { + if (this->disconnectedAt_.has_value()) { - // If we get a reconnected event when the room id is not set, we - // just connected for the first time. After receiving the first - // message from a channel, setRoomId is called and further - // invocations of this event will load recent messages. - return; + this->loadRecentMessagesReconnect(); + this->disconnectedAt_ = std::nullopt; } - - this->loadRecentMessagesReconnect(); }); // timers @@ -1111,6 +1106,24 @@ bool TwitchChannel::setLive(bool newLiveStatus) return true; } +void TwitchChannel::markDisconnectedNow() +{ + if (this->roomId().isEmpty()) + { + // we were never joined in the first place + return; + } + + if (this->disconnectedAt_.has_value()) + { + // don't overwrite prior timestamp since + // a reconnection hasn't happened yet + return; + } + + this->disconnectedAt_ = std::chrono::system_clock::now(); +} + void TwitchChannel::loadRecentMessages() { if (!getSettings()->loadTwitchMessageHistoryOnConnect) @@ -1163,7 +1176,9 @@ void TwitchChannel::loadRecentMessages() return; tc->loadingRecentMessages_.clear(); - }); + }, + getSettings()->twitchMessageHistoryLimit.getValue(), std::nullopt, + std::nullopt, false); } void TwitchChannel::loadRecentMessagesReconnect() @@ -1178,6 +1193,21 @@ void TwitchChannel::loadRecentMessagesReconnect() return; // already loading } + const auto now = std::chrono::system_clock::now(); + int limit = getSettings()->twitchMessageHistoryLimit.getValue(); + if (this->disconnectedAt_.has_value()) + { + // calculate how many messages could have occured + // while we were not connected to the channel + // assuming a maximum of 10 messages per second + const auto secondsSinceDisconnect = + std::chrono::duration_cast( + now - this->disconnectedAt_.value()) + .count(); + limit = + std::min(static_cast(secondsSinceDisconnect + 1) * 10, limit); + } + auto weak = weakOf(this); recentmessages::load( this->getName(), weak, @@ -1203,7 +1233,8 @@ void TwitchChannel::loadRecentMessagesReconnect() return; tc->loadingRecentMessages_.clear(); - }); + }, + limit, this->disconnectedAt_, now, true); } void TwitchChannel::refreshPubSub() diff --git a/src/providers/twitch/TwitchChannel.hpp b/src/providers/twitch/TwitchChannel.hpp index d9e5d3e4..5b1d628a 100644 --- a/src/providers/twitch/TwitchChannel.hpp +++ b/src/providers/twitch/TwitchChannel.hpp @@ -136,6 +136,12 @@ public: SharedAccessGuard accessRoomModes() const; SharedAccessGuard accessStreamStatus() const; + /** + * Records the current timestamp the channel was disconnected. + * This can be used to calculate the time spent disconnected after a successful reconnect + */ + void markDisconnectedNow(); + // Emotes std::optional bttvEmote(const EmoteName &name) const; std::optional ffzEmote(const EmoteName &name) const; @@ -200,6 +206,11 @@ public: */ std::shared_ptr getOrCreateThread(const MessagePtr &message); + /** + * This signal fires when the local user has joined the channel + **/ + pajlada::Signals::NoArgSignal joined; + // Only TwitchChannel may invoke this signal pajlada::Signals::NoArgSignal userStateChanged; @@ -353,6 +364,8 @@ private: int chatterCount_{}; UniqueAccess streamStatus_; UniqueAccess roomModes_; + std::optional> + disconnectedAt_{}; std::atomic_flag loadingRecentMessages_ = ATOMIC_FLAG_INIT; std::unordered_map> threads_;