Speed Reporting Fixes #13263

jagannatharjun · 2020-08-17T04:25:24Z

Basically aims to fix two bugs:

Total Upload speed in bottom bar do not fit with sum of partial Upload speeds #12459
speedplotview assumes speed is updated per second but the default value is 1500ms this causes lots of duplicate data in the calculation for the graph.

jagannatharjun · 2020-08-17T04:35:12Z

src/base/bittorrent/session.cpp

-        m_refreshEnqueued = false;
-    else
-        enqueueRefresh();
+    QTimer::singleShot(1000, this, [this]()


I would like to make refresh intervals for session stats 2s and remove 1-minute graph.

@Chocobo1 @glassez ?

@jagannatharjun Why not make the session stats interval configurable, or perhaps tied to the transfer list refresh interval?

Why not make the session stats interval configurable,

that will require a lot of work and is not worth it.

perhaps tied to the transfer list refresh interval?

it's already tied in current master and this PR basically untie it

I don't think we should provide this statistics in realtime, so I don't mind increasing this interval.
Is it used somewhere other than graphs, where it might be sensitive to such a change?

it's used in 2 more places, for mainwidow's tooltip and title text, and session statistics window, increasing interval should not be an issue I guess, but our speed plotting mechanism actually assumes a 1-second refresh interval so that requires some work which I already have a patch for. also Deluge also uses 2s interval for stats update

here's a question out of ignorance; is there anything preventing the Session object from being destructed while this timer is active? resulting in the callback being called and m_nativeSession being a dangling pointer.

here's a question out of ignorance; is there anything preventing the Session object from being destructed while this timer is active? resulting in the callback being called and m_nativeSession being a dangling pointer.

the function is called in the main loop, so afaik this won't be called after the Session object is destructed (the timer is tied to the lifetime of session object) or while it's being destructed since only one function can execute at a time.

I would like to make refresh intervals for session stats 2s and remove 1-minute graph.

I've been using this PR in my main setup for more than 2 months now and felt that 1-minute graph is actually useful plus graphs with 2s update intervals don't look good so in the latest revision, I changed the update interval back to 1s.

FranciscoPombal · 2020-08-30T01:06:23Z

@jagannatharjun

speedplotview assumes speed is updated per second but the default value is 1500ms this causes lots of duplicate data in the calculation for the graph.

Any metrics/benchmarks you can provide (e.g. before/after memory usage comparison or something)? How much is this "lots" we're talking about here?

jagannatharjun · 2020-08-30T07:08:22Z

Speed plot view uses a fixed-size cyclic buffer, so it can't be the culprit for high memory usage at least. This pr actually tries to fix step like graph plotting, you can try it yourself, change transfer list refresh interval to 1000 you will see much natural graphs

#13263 (comment) this may be helpful for performance.

glassez · 2020-09-02T17:31:44Z

src/base/bittorrent/session.cpp

@@ -2417,6 +2422,16 @@ void Session::configureListeningInterface()
    configureDeferred();
 }

+int Session::globalDownloadRate() const


Don't we overuse "global" in Session member names? Are there other download/upload rates so we can't just use downloadRate/uploadRate?

one could even argue that these aren't global. they are the session transfer rates. Presumably, if you'd have two Session objects, they could return different values here

these names were based on other Session members globalUploadSpeedLimit and globalDownloadSpeedLimit, do I still need to change these names (globalDownloadRate -> downloadRate and globalUploadRate -> uploadRate)?

also this comment #13263 (comment)

I'm not a stake-holder/approver in this repo, I'm just commenting.

IMO, all per-session indicators should have no "global" prefixes (e.g. Session::uploadRate() clearly shows that it's upload rate of all session torrents).

glassez · 2020-09-02T17:41:29Z

src/base/bittorrent/session.cpp

-        m_refreshEnqueued = false;
-    else
-        enqueueRefresh();
+    QTimer::singleShot(1000, this, [this]()


I don't think we should provide this statistics in realtime, so I don't mind increasing this interval.
Is it used somewhere other than graphs, where it might be sensitive to such a change?

glassez · 2020-09-02T17:54:21Z

src/base/bittorrent/session.cpp

@@ -4662,15 +4667,19 @@ void Session::handleStateUpdateAlert(const lt::state_update_alert *p)

        torrent->handleStateUpdate(status);
        updatedTorrents.push_back(torrent);
+
+        m_globalDownloadRate += torrent->downloadPayloadRate();
+        m_globalUploadRate += torrent->uploadPayloadRate();


Are you sure you can take into account updated torrents only? Is it impossible that some other torrents have non zero rates?

I don't know maybe @arvidn can help here?

in principle I believe it's possible for a torrent to have a non-zero transfer rate that doesn't change, and not be included in an update. It's extremely unlikely to happen though, since transfer rates are specified in bytes per second

btw, do we understand where the discrepancy is between the session transfer rates and the sum of the torrent rates? does the session not have a counter for payload-only transfers?

or is it just the update intervals being different?

do we understand where the discrepancy is between the session transfer rates and the sum of the torrent rates?

I think the problem is that the session stats update and torrent update can't be taken at the same "time point" causing discrepancy like these #12459 #8738 (the tab bottom speed is from post_session_stats while torrent speed are updated from post_torrent_updates)

is it just the update intervals being different

we try to always call post_torrent_updates and post_session_updates together but the problem (#12459 #8738) still persists.

xavier2k6 · 2020-09-15T11:24:15Z

@jagannatharjun would this close #8738 too?

arvidn · 2020-09-15T12:05:59Z

src/base/bittorrent/session.h

@@ -729,11 +731,12 @@ namespace BitTorrent

        int m_numResumeData = 0;
        int m_extraLimit = 0;
+        int m_globalDownloadRate = 0;
+        int m_globalUploadRate = 0;


I'm not an authority on this code base, but it seems like the global prefix is misleading here. Are there other transfer rates stored in the session? If anything, I would call these payloadDownloadRate and payloadUploadRate, since these counters only count the payload, not protocol overhead.

update SpeedPlotView on session stats update

Libtorrent stats and torrent updates can't be taken at same time, this causes out of sync speed reporting in different parts of GUI like in status bar, this commits adds two new fields in Session class i.e downloadRate and uploadRate which are calculated from torrent updates, they can be used to show global rates in GUI Use Session::uploadRate and Session::downloadRate in StatusBar and Main Window for reporting global rates

jagannatharjun · 2020-12-07T07:48:31Z

@glassez @Chocobo1 ping.

glassez · 2020-12-08T12:01:30Z

src/base/bittorrent/session.cpp

-        m_refreshEnqueued = false;
-    else
-        enqueueRefresh();
+    QTimer::singleShot(STATS_UPDATE_INTERVAL, this, [this]()


Why not use refreshInterval() for it too?

refreshInterval can be changed by the user and its default value is 1500ms. Speedplotview assumes speed is updated per second but actually before this PR, stats are updated on refreshInterval this caused lots of duplicate data in the calculation for the graph depending on the value of refreshInterval. So by updating stats per second, independent of refreshInterval I aims to solve this problem.

Previously session stats are also used to display global rates, but because of limitation in libtorrent they were not in-sync with the aggregate values of torrent speeds from transfer list (#12459) so with second commit we now calculate global download and upload rates by manually totalling individual torrent speed values.

With this session stats are now only used in graphs.

refreshInterval can be changed by the user and its default value is 1500ms. Speedplotview assumes speed is updated per second

So you don't think it makes sense to make Speedplotview dependent on the actual refresh interval (instead of some hardcoded value) to have it configurable?

So you don't think it makes sense to make Speedplotview dependent on the actual refresh interval (instead of some hardcoded value) to have it configurable?

It simplifies code and reduce efforts plus session stats are now only used in graphs.

It simplifies code and reduce efforts plus session stats are now only used in graphs.

Personally, I've never even changed "refresh interval". But apparently it might make sense for someone to make the refresh less frequent? So, doesn't it make the same sense to make all refresh stuff less frequent for the same reason?

session stats are now only used in graphs.

Only there? Really?
Then couldn't it make sense to make it available on demand, instead of constantly keep it refreshed at the app core level?

Personally, I've never even changed "refresh interval". But apparently it might make sense for someone to make the refresh less frequent? So, doesn't it make the same sense to make all refresh stuff less frequent for the same reason?

It seems a bit difficult to make SpeedPlotView refresh Interval aware.

Then couldn't it make sense to make it available on demand, instead of constantly keep it refreshed at the app core level?

querying stats via libtorrent, is an async operation so we will need to use the signals anyway, it may cause confusion down the line when someone else will need stats.

github-actions · 2021-08-12T00:46:28Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

github-actions · 2021-08-20T00:46:04Z

This PR was closed because it has been stalled for some time with no activity.

ghost · 2022-07-17T14:40:48Z

Useless bot closes important PRs but no one closes old stale issues -_-

glassez · 2022-07-17T15:02:18Z

Useless bot closes important PRs

It closes stale/inactive PRs, doesn't it?

no one closes old stale issues

Wasn't there an intention for the bot to close them? But then something went wrong. Or, as always, there was no agreement on what and when should be closed.

jagannatharjun commented Aug 17, 2020

View reviewed changes

FranciscoPombal added the GUI GUI-related issues/changes label Aug 17, 2020

xavier2k6 mentioned this pull request Aug 30, 2020

GUI performance with many thousands of torrents - transfer list, icons, and more #13304

Closed

FranciscoPombal requested review from glassez and Chocobo1 August 30, 2020 01:04

glassez reviewed Sep 2, 2020

View reviewed changes

arvidn reviewed Sep 15, 2020

View reviewed changes

xavier2k6 mentioned this pull request Sep 25, 2020

Improve startup #13348

Closed

jagannatharjun force-pushed the speed-i branch from 1e7fb1f to 2e6957e Compare October 3, 2020 06:48

jagannatharjun requested a review from glassez October 3, 2020 07:28

jagannatharjun added 2 commits December 7, 2020 13:12

Update session stats and torrent separately

7016cba

update SpeedPlotView on session stats update

jagannatharjun force-pushed the speed-i branch from 2e6957e to a25f185 Compare December 7, 2020 07:43

glassez reviewed Dec 8, 2020

View reviewed changes

jagannatharjun mentioned this pull request Dec 26, 2020

Make SpeedPlotView averager time aware #14054

Merged

github-actions bot added the Stale label Aug 12, 2021

github-actions bot closed this Aug 20, 2021

Speed Reporting Fixes #13263

Speed Reporting Fixes #13263

Conversation

jagannatharjun commented Aug 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagannatharjun Aug 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranciscoPombal commented Aug 30, 2020

jagannatharjun commented Aug 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagannatharjun Sep 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavier2k6 commented Sep 15, 2020

Choose a reason for hiding this comment

jagannatharjun commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagannatharjun Dec 24, 2020 • edited

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2021

github-actions bot commented Aug 20, 2021

ghost commented Jul 17, 2022

glassez commented Jul 17, 2022

jagannatharjun commented Aug 17, 2020 •

edited

jagannatharjun Aug 30, 2020 •

edited

jagannatharjun commented Aug 30, 2020 •

edited

jagannatharjun Sep 20, 2020 •

edited

jagannatharjun Dec 24, 2020 •

edited