perf: query fewer historical messages on reconnects #5001

iProdigy · 2023-12-04T03:46:49Z

Implements smaller ?limit on recent message query upon reconnects, as suggested in #4989

Note: system_clock is used instead of a strictly monotonic clock in anticipation of the since query parameter robotty/recent-messages2#286

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

src/providers/twitch/TwitchChannel.cpp

src/providers/irc/AbstractIrcServer.cpp

RAnders00 · 2023-12-05T02:01:48Z

The service at https://recent-messages.robotty.de now has a ?before and ?after parameter. Updated documentation can be found here.

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

src/providers/recentmessages/Impl.cpp

src/providers/recentmessages/Api.hpp

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

src/providers/twitch/TwitchChannel.cpp

RAnders00

I hope it's not too many changes, I was just trying to be thorough in all the places that I thought would require changes

src/providers/twitch/TwitchChannel.hpp

src/providers/twitch/TwitchChannel.cpp

RAnders00 · 2023-12-05T23:37:14Z

src/providers/twitch/TwitchChannel.cpp

+        const auto duration = std::ceil((now - this->disconnectedAt_) / 1000.0);
+        limit = std::min(static_cast<int>(duration) * 10,
+                         getSettings()->twitchMessageHistoryLimit.getValue());


Suggested change

const auto duration = std::ceil((now - this->disconnectedAt_) / 1000.0);

limit = std::min(static_cast<int>(duration) * 10,

getSettings()->twitchMessageHistoryLimit.getValue());

const auto disconnectDuration = now - this->disconnectedAt_;

const auto disconnectDurationSeconds = std::chrono::duration_cast<std::chrono::seconds>(disconnectDuration).count();

// At least 10 messages, even on very short reconnects that are < 1 full second

limit = std::min(std::max(disconnectDurationSeconds, 1) * 10, limit);

i think we actually always want to round up to the nearest int (i.e., treat 999ms as 1s, treat 1100ms as 2s - in the 1100ms case, 10 messages could occur at t=0 and 10 at t=1100)

src/providers/twitch/TwitchChannel.cpp

src/providers/recentmessages/Impl.cpp

src/providers/recentmessages/Impl.hpp

src/providers/twitch/TwitchChannel.cpp

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

src/providers/recentmessages/Api.hpp

github-actions

clang-tidy made some suggestions

src/providers/twitch/TwitchChannel.cpp

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.hpp

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

Co-authored-by: Ruben Anders <[email protected]>

github-actions

clang-tidy made some suggestions

src/providers/recentmessages/Api.cpp

…meter default values

…-reconnect-load

Co-authored-by: Ruben Anders <[email protected]>

pajlada

Looks good to me - can you take a look at the changes I've made to see if they make sense @iProdigy?
After that, I'll go ahead and merge this in.

iProdigy · 2023-12-09T18:09:54Z

Changes look good to me! thank you @pajlada and @RAnders00

src/providers/twitch/TwitchChannel.hpp

Co-authored-by: Ruben Anders <[email protected]>

pajlada · 2023-12-09T18:46:09Z

I'm going to merge this in, but one thing I thought about is the use case for when someone puts their computer to sleep with Chatterino running. Will Chatterino only request 10-20 messages upon the computer waking up in this case?

RAnders00 · 2023-12-09T20:09:21Z

To be honest we could have skipped the entire ?limit parameter calculation. The before and after parameters do everything for us, and I don't need the ?limit for server performance, filtering by time range without limit or with a high limit is just as efficient, it's all about the number of messages returned.

pajlada · 2023-12-09T20:19:51Z

I made #5009 to track any follow-up changes related to sleep

iProdigy · 2023-12-09T20:37:17Z

To be honest we could have skipped the entire ?limit parameter calculation. The before and after parameters do everything for us, and I don't need the ?limit for server performance, filtering by time range without limit or with a high limit is just as efficient, it's all about the number of messages returned.

even if we omitted limit, the sleep issue would remain since it's likely that markDisconnectedNow occurs after the computer wakes from sleep

perf: query fewer historical messages on reconnects

343cd70

github-actions bot reviewed Dec 4, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved

iProdigy commented Dec 4, 2023

View reviewed changes

src/providers/irc/AbstractIrcServer.cpp Outdated Show resolved Hide resolved

feat: specify before and after timestamps

469f41c

github-actions bot reviewed Dec 5, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

iProdigy and others added 2 commits December 4, 2023 19:15

chore: update changelog

f2c7f71

feat: add jitter to historical message requests

a5bd088

iProdigy marked this pull request as ready for review December 5, 2023 19:01

github-actions bot reviewed Dec 5, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

Merge branch 'master' into perf/recent-message-reconnect-load

9ea6c20

RAnders00 reviewed Dec 5, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Outdated Show resolved Hide resolved

RAnders00 reviewed Dec 5, 2023

View reviewed changes

src/providers/recentmessages/Impl.cpp Outdated Show resolved Hide resolved

iProdigy added 2 commits December 5, 2023 12:56

docs: clarify default timestamp behavior

138efd2

refactor: ensure timestamp is stored as 64-bit

fb6a152

iProdigy requested a review from RAnders00 December 5, 2023 21:39

Nerixyz reviewed Dec 5, 2023

View reviewed changes

src/providers/recentmessages/Api.hpp Outdated Show resolved Hide resolved

github-actions bot reviewed Dec 5, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Show resolved Hide resolved

src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved

RAnders00 suggested changes Dec 6, 2023

View reviewed changes

iProdigy marked this pull request as draft December 6, 2023 22:02

refactor: use optional time points

077aa85

github-actions bot reviewed Dec 6, 2023

View reviewed changes

fix: delay historical message query until rejoined

e6e293e

github-actions bot reviewed Dec 6, 2023

View reviewed changes

src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved

iProdigy added 2 commits December 6, 2023 14:48

chore: use pointer accessor

1b5b9fb

chore: use add chrono headers

45e6093

github-actions bot reviewed Dec 6, 2023

View reviewed changes

src/providers/recentmessages/Api.hpp Outdated Show resolved Hide resolved

src/providers/recentmessages/Api.hpp Outdated Show resolved Hide resolved

src/providers/recentmessages/Api.hpp Outdated Show resolved Hide resolved

chore: add header for optional

2d490a7

github-actions bot reviewed Dec 6, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Show resolved Hide resolved

Apply suggestions from code review

4a1cc13

Co-authored-by: Ruben Anders <[email protected]>

refactor: make limit a required arg

82ecf4d

github-actions bot reviewed Dec 7, 2023

View reviewed changes

src/providers/recentmessages/Api.cpp Show resolved Hide resolved

src/providers/recentmessages/Api.cpp Show resolved Hide resolved

iProdigy marked this pull request as ready for review December 7, 2023 02:40

fix: avoid double query when reconnect is requested before initial join

855639f

pajlada self-requested a review December 8, 2023 23:24

pajlada and others added 10 commits December 9, 2023 00:24

Merge branch 'master' into perf/recent-message-reconnect-load

ebb1446

nit: Remove recentmessages::load's parameter default values

6ca8e92

nit: Remove recentmessages::detail::constructRecentMessagesUrl para…

1b48f69

…meter default values

nit: recentmessages impl: remove unused settings include

bff18bf

nit: Don't respect after & before query params in env variable URL

12bf88f

Merge remote-tracking branch 'origin/master' into perf/recent-message…

dc9985d

…-reconnect-load

nit: rename duration to secondsSinceDisconnect

3d7d65c

nit: Remove now-unused Channel::connected signal

a2ee3c2

Update changelog entry

3aed60c

nit: Update comment for TwitchChannel::markDisconnectedNow

41b3c74

Co-authored-by: Ruben Anders <[email protected]>

pajlada approved these changes Dec 9, 2023

View reviewed changes

Merge branch 'master' into perf/recent-message-reconnect-load

94e78e3

RAnders00 suggested changes Dec 9, 2023

View reviewed changes

src/providers/twitch/TwitchChannel.hpp Outdated Show resolved Hide resolved

docs: fix typo in method code comment

0f8131f

Co-authored-by: Ruben Anders <[email protected]>

pajlada merged commit 13dc306 into Chatterino:master Dec 9, 2023
18 of 20 checks passed

iProdigy deleted the perf/recent-message-reconnect-load branch December 9, 2023 20:36

pajlada mentioned this pull request Dec 10, 2023

Message history request after Twitch RECONNECT message is unecessarily large #4989

Closed

4 tasks

ilya-zlobintsev mentioned this pull request Jan 30, 2024

Add developer_name appdata field ilya-zlobintsev/chatterino2#3

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: query fewer historical messages on reconnects #5001

perf: query fewer historical messages on reconnects #5001

iProdigy commented Dec 4, 2023 •

edited

Loading

github-actions bot left a comment

RAnders00 commented Dec 5, 2023

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

RAnders00 left a comment

RAnders00 Dec 5, 2023

iProdigy Dec 6, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

pajlada left a comment

iProdigy commented Dec 9, 2023

pajlada commented Dec 9, 2023

RAnders00 commented Dec 9, 2023 •

edited

Loading

pajlada commented Dec 9, 2023

iProdigy commented Dec 9, 2023

-        const auto duration = std::ceil((now - this->disconnectedAt_) / 1000.0);
-        limit = std::min(static_cast<int>(duration) * 10,
-                         getSettings()->twitchMessageHistoryLimit.getValue());
+        const auto disconnectDuration = now - this->disconnectedAt_;
+        const auto disconnectDurationSeconds = std::chrono::duration_cast<std::chrono::seconds>(disconnectDuration).count();
+        // At least 10 messages, even on very short reconnects that are < 1 full second
+        limit = std::min(std::max(disconnectDurationSeconds, 1) * 10, limit);

perf: query fewer historical messages on reconnects #5001

perf: query fewer historical messages on reconnects #5001

Conversation

iProdigy commented Dec 4, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

RAnders00 commented Dec 5, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

RAnders00 left a comment

Choose a reason for hiding this comment

RAnders00 Dec 5, 2023

Choose a reason for hiding this comment

iProdigy Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

pajlada left a comment

Choose a reason for hiding this comment

iProdigy commented Dec 9, 2023

pajlada commented Dec 9, 2023

RAnders00 commented Dec 9, 2023 • edited Loading

pajlada commented Dec 9, 2023

iProdigy commented Dec 9, 2023

iProdigy commented Dec 4, 2023 •

edited

Loading

iProdigy Dec 6, 2023 •

edited

Loading

RAnders00 commented Dec 9, 2023 •

edited

Loading