-
Notifications
You must be signed in to change notification settings - Fork 36.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Qt] simple mempool info in debug window #6979
[Qt] simple mempool info in debug window #6979
Conversation
<widget class="QLabel" name="labelDebugLogfile"> | ||
<widget class="QLabel" name="label_3"> | ||
<property name="text"> | ||
<string>Current amount of transactions</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current number of transactions
Concept ACK |
@@ -77,6 +77,8 @@ public Q_SLOTS: | |||
void setNumConnections(int count); | |||
/** Set number of blocks and last block date shown in the UI */ | |||
void setNumBlocks(int count, const QDateTime& blockDate); | |||
/** Set size (amount of transactions and memory usage) of the mempool in the UI */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amount -> number
Can you please change Mempool in the above screenshot to Memory pool? I'd like to see also the graph of the memory pool - similar to the Network Traffic tab... |
@paveljanik: thanks for the review. Yes. The mempool chart is also on my list of things i'd like to do (... and that list is already very large). |
conceptACK |
Nit's addressed with two squashme commits. |
Random suggestion: under "Block chain", put a " Database cache siE" entry
that is pcoinsTip->GetDynamicUsage()" ?
|
@sipa: good point. But because of the window height (I try not to enlarge), maybe the database cache size fits better in a new "limits" (or "usage") screen/graph. There i could imaging a mempool size/memusage chart as well as the database-cache (chart and current value). |
Fair enough, keep the pull request as-is then.
|
if (dynUsage < 1024*1024) | ||
ui->mempoolSize->setText(QString::number(dynUsage/1024.0, 'f', 2) + " KB"); | ||
else | ||
ui->mempoolSize->setText(QString::number(dynUsage/1024.0/1024.0, 'f', 2) + " MB"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like all those MB, MiB inconsistencies. At least make it consistent with the parameter which sets the mempool size. (Which is not MiB iirc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using MB (1e6 bytes) unless there is a convincing reason to use MiB (2^20 bytes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some people refer to 2^20 bytes as "MB", so at least MiB is clear what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's used less commonly, so MB is fine as well. Just be consistent, which is what the NIT is complaining about. You can't parse the command line in MB (1e6) and then display MB (10^20)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In bitcoin core we stick to SI units and use MB, which is defined as 1000000, not 1024*1024, MiB, etc if we do that that's a mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, Concept ACK. |
Compile log compared to the master:
|
Mention use of SI units and prefixes. This has always been the case, but make it explicit after discussion in bitcoin#6979.
@paveljanik: these warning are well known and they are unrelated to this PR. Have also plans to fix the qt id/name conflicts (warnings) soon. |
Switched to MB / SI units (1000^2 for MB). |
@jonasschnelli I compared the log of the master with the log of the master with this PR. I do not see such warnings in the master builds... I have not investigated from where they come though. |
</widget> | ||
</item> | ||
<item row="8" column="0"> | ||
<widget class="QLabel" name="label_11"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pointed out by @paveljanik label_11
seems already in use: C.f. $ grep -r 'label_11' src/qt/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking: Is it encouraged to use (name)_i++
for references? I'd prefer something like label_memp_whatever
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Agree with that. It was already named label_11
before this PR. The diff looks a bit strange. But i have fixed this now.
9824fb0
to
3573481
Compare
Right. There where QT name/id conflicts (some introduced over this PR), although, Qt auto-increments conflicting IDs and because we don't access them through the code, it wouldn't be a problem. Added a squashme-commit with a fix. |
3573481
to
67b1393
Compare
Here are the binaries if someone likes to test this: https://bitcoin.jonasschnelli.ch/pulls/6979/ |
Memory usage is continuously updated here, but always ends with |
@paveljanik: if the memory usage is grater than 1000000 bytes it will use "MB" and there the precision of two decimal places make sense IMO. |
67b1393
to
96a0007
Compare
Fixed the size_t to float conversion. I accidentally dropped the 1000**.0**/1000000**.0** in the arithmetic. |
ACK |
I was wondering how you get the first screenshot 👍 |
I think somewhere the |
I do not think so (division is evaluated first). Maybe his screenshot is from the same code from which you did the screenshot. |
96a0007
to
7dc0835
Compare
@@ -88,6 +89,16 @@ QDateTime ClientModel::getLastBlockDate() const | |||
return QDateTime::fromTime_t(Params().GenesisBlock().GetBlockTime()); // Genesis block's time of current network | |||
} | |||
|
|||
size_t ClientModel::getMempoolSize() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Isn't this long
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Right, .. the map's size() itself is size_t
, but the CTxMempool uses long size()
in its interface.
Fixed.
7dc0835
to
70a6ed8
Compare
utACK c197798 |
0243ab8
to
88865f1
Compare
88865f1
to
c197798
Compare
Teseted ACK c197798. Code looks clean. |
Tested ACK |
c197798 [Qt] simple mempool info in debug window (Jonas Schnelli)
Having the mempool size in the debug window can be useful.
This PR also moves the "open debug log" button to the right side of the debug window.
The stats gets updated over the same TryLock that is used for the bandwidth graph (no additional blocking is required).
It will ~look like this:
