Page MenuHomePhabricator

Add DynamicMemoryUsage() to LevelDB
ClosedPublic

Authored by fpelliccioni on Mon, Aug 26, 18:32.

Details

Reviewers
Fabien
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC8b217943d251: Add DynamicMemoryUsage() to LevelDB
Summary
Test Plan
make check

Run bitcoind with -debug=leveldb and check the log file for the LEVELDB category and the following pattern:
WriteBatch memory usage: db=..., before=...MiB, after=...MiB
Verify that printed sizes (before and after) make sense.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Mon, Aug 26, 18:32
Owners added a reviewer: Restricted Owners Package.Mon, Aug 26, 18:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Aug 26, 18:32
fpelliccioni updated this revision to Diff 10961.Mon, Aug 26, 20:01

added Linter exception

Fabien requested changes to this revision.Tue, Aug 27, 08:55

Your test plan is not testing for the change, please update it.

src/dbwrapper.cpp
175 ↗(On Diff #10961)

I don't get why memory usage is a double.

My understanding is that it is an integer division which is casted into a double then printed with 1 decimal.
This decimal value will always be 0 in this case, and you get the overhead of float parsing/printing for no value.

I tested this to see whether I was missing something and couldn't get any value with a non-zero decimal.

This revision now requires changes to proceed.Tue, Aug 27, 08:55
fpelliccioni added inline comments.Tue, Aug 27, 16:24
src/dbwrapper.cpp
175 ↗(On Diff #10961)

Yes, you are right, it is a bug.
I knew this problem beforehand, but I understand that I have to wait for D3948 to be approved to apply the fix.
Maybe I am wrong... I have to improve my understanding about the ABC workflow.

It is fixed here:

https://github.com/jasonbcox/bitcoin/commit/e80716d3b

(Pedantic: strictly speaking it is not a Cast it is a Floating–integral (implicit) conversion)

fpelliccioni edited the test plan for this revision. (Show Details)Tue, Aug 27, 18:50
fpelliccioni edited the summary of this revision. (Show Details)Tue, Aug 27, 20:03
Fabien accepted this revision.Tue, Aug 27, 20:04
This revision is now accepted and ready to land.Tue, Aug 27, 20:04
This revision was automatically updated to reflect the committed changes.