Slow memory leak in v22.0?

sangaman commented at 4:25 pm on March 12, 2022: contributor

I’ve been noticing gradually increasing memory usage bitcoin core v22.0. I’ve been running this node for a couple of years with the same hardware and general setup and have only noticed this in the past few months, around the time I upgraded to v22, which makes me suspect it may be related.

I’m seeing bitcoind memory usage slowly climb over weeks/months of uptime. Right now it’s at 3.4 GB memory usage and as I recall I’ve seen it over 5 GB before in an incident where it caused the machine to run out of RAM, which is when I first noticed this. If I restart bitcoind, RAM usage goes back to a normal level and stays there for a while.

I do recall seeing a lot of BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications messages in the logs when RAM usage was over 5GB and my node was barely responsive to RPC calls, but I’m not sure if this has anything to do with the issue.

I’m using dbcache=1024 which would explain some of the RAM usage but not all of it.

Other config settings that may be relevant:

 0txindex=1
 1peerblockfilters=1
 2blockfilterindex=1
 3
 4maxconnections=100
 5
 6rpcthreads=64
 7rpcworkqueue=256
 8rpcservertimeout=120
 9
10zmqpubrawblockhwm=100000
11zmqpubrawtxhwm=100000

Otherwise, I’m using this node to run an lnd node (with zmq block/tx notifications) and electrumx server. There’s a medium-sized watch-only wallet that’s loaded. There’s also a small script I run with blocknotify.

This is all running on an RPI4 with 8GB RAM, using raspbian buster w/ the OS and block indexes on an SSD.

I’d be interested if others have seen anything similar out of their nodes or if perhaps this RAM usage is normal given how I’m using it, but I am pretty sure this wasn’t happening a year ago with similar usage patterns.

fanquake added the label Resource usage on Mar 12, 2022

ajtowns commented at 9:06 am on March 15, 2022: contributor

I think I’ve seen the same thing; but mostly for me it just looks like “bitcoind has a bunch of memory in swap and it takes a while to deallocate when shutting down” so I haven’t been able to get any insight into what’s actually going on. I think the same memory leak may show up in liquid/elements, but with higher severity (due to the higher block rate)

EDIT: maybe worth noting that the “takes a while to deallocate” occurs after Shutdown() has finished

maflcko commented at 9:20 am on March 15, 2022: member

Maybe valgrind --tool=massif bitcoind could help here?

sangaman commented at 9:31 pm on March 15, 2022: contributor

I went to check on my node today expecting higher RAM usage than before, but it was actually down to 1.9 GB. Puzzled, I checked and it looks like the node actually shutdown 2 days ago (and systemd restarted it automatically). Looking at the logs, there are tons of these BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications. Here’s what my logs look like.

  02022-03-13T04:48:01Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
  12022-03-13T04:48:01Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
  22022-03-13T04:53:05Z UpdateTip: new best=00000000000000000009e50a3c45c26a64cfe507b5abee9432460ed3d85c6855 height=727101 version=0x20200000 log2_work=93.398160 tx=717198865 date='2022-03-13T04:51:03Z' progress=1.000000 cache=32.2MiB(147906txo)
  32022-03-13T04:53:05Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
  42022-03-13T04:53:05Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
  52022-03-13T04:54:48Z UpdateTip: new best=000000000000000000058b74712cac983dd446c6b2c5e078db9975c6638e86fd height=727102 version=0x20000000 log2_work=93.398173 tx=717199047 date='2022-03-13T04:52:40Z' progress=1.000000 cache=32.3MiB(148339txo)
  62022-03-13T04:54:48Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
  72022-03-13T04:54:48Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
  82022-03-13T05:03:47Z UpdateTip: new best=0000000000000000000516fd3c3ecfb22e47ba8ed8c29163602230aee526d147 height=727103 version=0x20200000 log2_work=93.398186 tx=717199511 date='2022-03-13T04:57:22Z' progress=0.999999 cache=32.5MiB(150501txo)
  92022-03-13T05:03:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 102022-03-13T05:03:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 112022-03-13T05:03:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 122022-03-13T05:03:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 132022-03-13T05:03:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 142022-03-13T05:03:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 152022-03-13T05:10:59Z UpdateTip: new best=00000000000000000009554288443c39f85f3e531e9e60c48db2d01a22585b81 height=727104 version=0x2000e004 log2_work=93.398199 tx=717200040 date='2022-03-13T05:02:51Z' progress=0.999998 cache=32.8MiB(152399txo)
 162022-03-13T05:10:59Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 172022-03-13T05:10:59Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 182022-03-13T05:19:14Z UpdateTip: new best=0000000000000000000358f170fd5d78721254c9f3cad519155d36eaef8c1c63 height=727105 version=0x20004000 log2_work=93.398212 tx=717200804 date='2022-03-13T05:10:56Z' progress=0.999998 cache=33.4MiB(157443txo)
 192022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 202022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 212022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 222022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 232022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 242022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 252022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 262022-03-13T05:19:15Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 272022-03-13T05:19:41Z UpdateTip: new best=00000000000000000002bc5d2f34d9bb7c92d2e2c6f2a578ed704f7498cf4d53 height=727106 version=0x3fffe000 log2_work=93.398226 tx=717200853 date='2022-03-13T05:12:02Z' progress=0.999998 cache=33.4MiB(157649txo)
 282022-03-13T05:19:42Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 292022-03-13T05:22:21Z UpdateTip: new best=0000000000000000000066ff7813973ee5068a0ff061f42901360871086a5025 height=727107 version=0x3fffe000 log2_work=93.398239 tx=717201297 date='2022-03-13T05:16:01Z' progress=0.999999 cache=33.6MiB(158708txo)
 302022-03-13T05:22:22Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 312022-03-13T05:22:22Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 322022-03-13T05:31:47Z UpdateTip: new best=000000000000000000025ce8ab1c48e53f8cb2d5000807cf45ec18e543773f84 height=727108 version=0x2cbd0000 log2_work=93.398252 tx=717202203 date='2022-03-13T05:24:43Z' progress=0.999999 cache=33.9MiB(161187txo)
 332022-03-13T05:31:49Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 342022-03-13T05:31:49Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 352022-03-13T05:31:49Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 362022-03-13T05:31:49Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 372022-03-13T05:31:49Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 382022-03-13T05:31:49Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 392022-03-13T05:37:23Z UpdateTip: new best=00000000000000000003be53d87aaf7489dc53d3c36fc36ea7b42d541a19c45f height=727109 version=0x22c18000 log2_work=93.398265 tx=717202876 date='2022-03-13T05:31:32Z' progress=0.999999 cache=34.1MiB(163020txo)
 402022-03-13T05:37:23Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 412022-03-13T05:37:23Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 422022-03-13T05:37:23Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 432022-03-13T05:37:23Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 442022-03-13T05:37:23Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 452022-03-13T05:37:23Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 462022-03-13T05:37:41Z UpdateTip: new best=00000000000000000005464d4bd8530348a5b08eb2362f0f621213ca04873852 height=727110 version=0x27ffe004 log2_work=93.398278 tx=717202909 date='2022-03-13T05:31:53Z' progress=0.999999 cache=34.1MiB(163122txo)
 472022-03-13T05:37:42Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 482022-03-13T05:41:20Z UpdateTip: new best=000000000000000000045ec5d2b3999a4336dd48724fed813ae8682868bf94b6 height=727111 version=0x20c00000 log2_work=93.398291 tx=717203383 date='2022-03-13T05:36:18Z' progress=0.999999 cache=34.3MiB(164597txo)
 492022-03-13T05:41:21Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 502022-03-13T05:41:21Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 512022-03-13T05:41:21Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 522022-03-13T05:41:25Z New outbound peer connected: version: 70015, blocks=727111, peer=656929 (outbound-full-relay)
 532022-03-13T05:41:48Z New outbound peer connected: version: 70016, blocks=727111, peer=657055 (outbound-full-relay)
 542022-03-13T05:42:44Z New outbound peer connected: version: 70016, blocks=727111, peer=657066 (outbound-full-relay)
 552022-03-13T05:58:47Z UpdateTip: new best=00000000000000000008ec489c496cca28028dfe29fec7c1cbf3c43dc7156312 height=727112 version=0x20c00004 log2_work=93.398304 tx=717204079 date='2022-03-13T05:42:48Z' progress=0.999997 cache=34.6MiB(167004txo)
 562022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 572022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 582022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 592022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 602022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 612022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 622022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 632022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 642022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 652022-03-13T05:58:47Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 662022-03-13T05:58:48Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 672022-03-13T05:58:48Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 682022-03-13T05:58:48Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 692022-03-13T05:58:48Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 702022-03-13T05:59:30Z New outbound peer connected: version: 70016, blocks=727112, peer=657227 (outbound-full-relay)
 712022-03-13T06:00:03Z New outbound peer connected: version: 70016, blocks=727112, peer=657238 (outbound-full-relay)
 722022-03-13T06:00:05Z New outbound peer connected: version: 70016, blocks=727112, peer=657239 (outbound-full-relay)
 732022-03-13T06:00:13Z New outbound peer connected: version: 70015, blocks=727112, peer=657240 (outbound-full-relay)
 742022-03-13T06:08:20Z New outbound peer connected: version: 70016, blocks=727112, peer=657341 (outbound-full-relay)
 752022-03-13T06:41:11Z UpdateTip: new best=00000000000000000005fda11e6f284235a2c2cfcf74303e9610a40b5e489664 height=727113 version=0x345ca000 log2_work=93.398317 tx=717206214 da
 76te='2022-03-13T06:16:35Z' progress=0.999995 cache=36.0MiB(178695txo)
 772022-03-13T06:41:25Z Potential stale tip detected, will try using extra outbound peer (last tip update: 2549 seconds ago)
 782022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 792022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 802022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 812022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 822022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 832022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 842022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 852022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 862022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 872022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 882022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 892022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 902022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 912022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 922022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 932022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 942022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 952022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 962022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 972022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 982022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
 992022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
1002022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
1012022-03-13T06:41:25Z BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications
1022022-03-13T06:41:26Z New outbound peer connected: version: 70015, blocks=727116, peer=657444 (outbound-full-relay)
1032022-03-13T06:41:47Z New outbound peer connected: version: 70016, blocks=727116, peer=657578 (outbound-full-relay)
1042022-03-13T06:41:47Z New outbound peer connected: version: 70016, blocks=727116, peer=657579 (outbound-full-relay)
1052022-03-13T06:42:04Z New outbound peer connected: version: 70015, blocks=727116, peer=657584 (outbound-full-relay)
1062022-03-13T06:42:25Z New outbound peer connected: version: 70015, blocks=727117, peer=657591 (outbound-full-relay)
1072022-03-13T06:42:41Z New outbound peer connected: version: 70016, blocks=727117, peer=657596 (outbound-full-relay)
1082022-03-13T06:43:00Z New outbound peer connected: version: 70015, blocks=727117, peer=657598 (outbound-full-relay)
1092022-03-13T06:43:04Z New outbound peer connected: version: 70016, blocks=727117, peer=657599 (outbound-full-relay)
1102022-03-13T06:43:40Z New outbound peer connected: version: 70016, blocks=727117, peer=657607 (outbound-full-relay)
1112022-03-13T06:44:36Z New outbound peer connected: version: 70016, blocks=727117, peer=657614 (outbound-full-relay)
1122022-03-13T06:44:37Z New outbound peer connected: version: 70016, blocks=727117, peer=657615 (outbound-full-relay)
1132022-03-13T06:44:38Z New outbound peer connected: version: 70015, blocks=727117, peer=657616 (outbound-full-relay)
1142022-03-13T06:45:07Z New outbound peer connected: version: 70016, blocks=727117, peer=657618 (block-relay-only)
1152022-03-13T06:45:43Z New outbound peer connected: version: 70015, blocks=727117, peer=657630 (outbound-full-relay)

That’s the last log message before it switches to startup logs. Looks like peers started dropping my node, I’m guessing because it was unresponsive, and so it was making a lot of outbound connections, and looks like block syncing crawled to a stop.

I’ll try to keep tabs on things and keep this issue updated if I see anything else interesting.

mzumsande commented at 11:13 pm on March 15, 2022: contributor

Looking at the logs, there are tons of these BlockUntilSyncedToCurrentChain: txindex is catching up on block notifications.

These are likely caused by multiple parallel getrawtransaction RPC calls (probably from the lnd node) - if the txindex is enabled but hasn’t managed to process the newest block yet (as a result of a general slowness / unresponsiveness due to memory shortage), it will log this message. So I think that these are probably not the cause of the memory problem, but appear as a result of it.

sangaman commented at 11:27 pm on March 15, 2022: contributor

These are likely caused by multiple parallel getrawtransaction RPC calls (probably from the lnd node) - if the txindex is enabled but hasn’t managed to process the newest block yet (as a result of a general slowness / unresponsiveness due to memory shortage), it will log this message. So I think that these are probably not the cause of the memory problem, but appear as a result of it.

Thanks, iirc when I tried making rpc calls via bitcoin-cli one of the last times this happened the command would hang while I saw that BlockUntilSyncedToCurrentChain, so that makes sense.

cshintov commented at 6:00 am on May 4, 2023: none

Hi facing similar issue. I’m running an rpc node. My memory usage grows beyond 36GB. I only see this problem when setting rpcthreads parameter. Without rpcthreads the usage doesn’t grow beyond 4 GB.

Did you figure out why the leak?

mzumsande commented at 4:51 pm on May 4, 2023: contributor

Hi facing similar issue. I’m running an rpc node.

Also 22.0? Which value of rpcthreads are you using?

sangaman commented at 5:12 pm on May 4, 2023: contributor

FYI, I am now using v24.0.1 with rpcthreads=64 as mentioned above, and I still see this issue although it still appears to be slow and unpredictable. I just checked and bitcoind is currently using close to 4 GB RAM, which is a lot higher than it would be if I’d restarted the node within the past few hours or even days.

I increased the rpcthreads setting to make sure bitcoind plays nice with lnd, but I don’t expect that I actually need it that high. I can switch to the default rpcthreads value and see if this issue goes away, I’ll report back later.

willcl-ark commented at 5:16 pm on May 4, 2023: member

Thanks @sangaman. Additionally, do you have any estimates on how many rpcs you are making per time to the node?

sangaman commented at 5:20 pm on May 4, 2023: contributor

Hmm I’m not exactly sure. This bitcoind node is primarily used to serve an lnd node as well as an electrum server and mempool.space block explorer, although the latter two get very light usage aside from staying in sync with the chain. If you think it’d be helpful I can try to turn on debug logs and see how many rpc calls are made over a certain period of time.

cshintov commented at 6:00 pm on May 4, 2023: none

I’m running 24.0.1 and I tried with various rpcthreads values 50, 100, 150.

I could solve the issue by setting the below env var.

By default, since glibc 2.10, the C library will create up to two heap arenas per core. This is known to cause excessive memory usage in some scenarios. To avoid this we have to set

MALLOC_ARENA_MAX=1

without this the usage grows beyond 36GB, with this it stays under 2GB normally.

ref: https://github.com/bitcoin/bitcoin/blob/master/doc/reduce- memory.md#linux-specific

sangaman commented at 7:18 pm on May 4, 2023: contributor

Thank you @cshintov ! I’ll try this env var out and see if it solves my issue without having to change my config.

sangaman commented at 2:06 pm on May 12, 2023: contributor

So far my results with MALLOC_ARENA_MAX=1 seem to be good, I’ve been comfortably under 2GB RAM every time I’ve checked. I added it as an env var in my systemd configuration which I copied and modified from the one in this repo, I figure it ought to have that env var as well. I’ll open a PR to add it.

sangaman referenced this in commit 5b1726ec0d on May 12, 2023

willcl-ark commented at 2:24 pm on May 12, 2023: member

I think setting this to 1 will give significantly worse performance for most users though?

Seems like it would be better to add it to the documentation if anything…

sangaman referenced this in commit 7cbcb55d82 on May 12, 2023

sangaman referenced this in commit 3dd8faffca on May 12, 2023

sangaman referenced this in commit 0cd0eec466 on May 12, 2023

sangaman commented at 3:18 pm on May 12, 2023: contributor

Seems like it would be better to add it to the documentation if anything…

It is in the documentation already at https://github.com/bitcoin/bitcoin/blob/master/doc/reduce-memory.md.

I think setting this to 1 will give significantly worse performance for most users though?

Per the documentation above that wouldn’t be the case although I can’t say for sure. I certainly didn’t notice any decreased performance when setting this env var.

The behavior was introduced to increase CPU locality of allocated memory and performance with concurrent allocation, so this setting could in theory reduce performance. However, in Bitcoin Core very little parallel allocation happens, so the impact is expected to be small or absent.

willcl-ark commented at 3:13 pm on April 10, 2024: member

Is this still a problem with v26.0 (or later)?

maflcko commented at 10:14 am on May 8, 2024: member

Not sure what can be done here. It would be good to use a tool to debug this, e.g. #24542 (comment), if it still happens.

willcl-ark commented at 3:09 pm on May 10, 2024: member

OK, I think I have pretty much identified the root cause here.

Calling blockToJSON on a maximum-sized block allocates 70-80MB on the heap during runtime.

There is then an additional 15-20MB allocated for strJSON.

After these allocations are made they then seem to be re-used when possible in the future, so long as subsequent blocks are small enough to fit. This totals up to the ~100MB RSS increase observed sometimes when getting blocks via REST.

In the event that blocks are fetched in series in increasing sizes, repeated allocations (of increasing sizes) may be made, causing even higher apparent resource usage.

The cause seems to be two-fold:

UniValue’s are not a particularly compact representation (can’t do that much about this, except perhaps we could write to string directly for this method.)
The UniValue created inside blockToJSON is ~40MB worst-case, but it is then copied into a second UniValue, doubling the required heap size.

It appears that this has been investgated previously (see also here) in search of better speed, but having a move semantic for UniValue would also shave off half of this function’s allocation requirement.

Thanks @stickies-v for helping me investigate

stickies-v commented at 4:04 pm on May 10, 2024: contributor

but it is then copied into a second UniValue, doubling the required heap size.

And if I understand correctly, it is then copied again here since UniValue doesn’t have a move constructor. So improving UniValue move semantics seems to make sense here indeed.

willcl-ark commented at 7:50 pm on May 10, 2024: member

Discovered another attempt to avoid copies from @maflcko #25429 while researching further.

Ref my previous test in #30052, I have a branch with some improvements (heavily “insipred” by @martinus ’s commit https://github.com/jgarzik/univalue/pull/79/commits/e9109e2a04b47472fe45a9cae3c6b41c245a6db9) which limit allocations to about 60-70MB:

This is about what I’d expect to be the minimum size based on the allocation for the JSON string, and the UniValue Object.

maflcko commented at 11:44 am on May 13, 2024: member

And if I understand correctly, it is then copied again here since UniValue doesn’t have a move constructor. So improving UniValue move semantics seems to make sense here indeed.

Can you explain this? It has a move constructor, at least the following compiles for me:

 0diff --git a/src/univalue/lib/univalue.cpp b/src/univalue/lib/univalue.cpp
 1index 656d2e8203..b3a33f36ac 100644
 2--- a/src/univalue/lib/univalue.cpp
 3+++ b/src/univalue/lib/univalue.cpp
 4@@ -125,6 +125,7 @@ void UniValue::pushKVEnd(std::string key, UniValue val)
 5 
 6 void UniValue::pushKV(std::string key, UniValue val)
 7 {
 8+static_assert(std::is_move_constructible_v<UniValue>);
 9     checkType(VOBJ);
10 
11     size_t idx;

The UniValue created inside blockToJSON is ~40MB worst-case, but it is then copied into a second UniValue, doubling the required heap size.

I presume this can be fixed by adding a std::move, but I haven’t tested it.

martinus commented at 12:34 pm on May 13, 2024: contributor

The UniValue created inside blockToJSON is ~40MB worst-case, but it is then copied into a second UniValue, doubling the required heap size.

I had a PR somewhere where I removed the copy constructor and made it mandatory to call a .copy(). Then you’d get a compile error wherever a copy happens, and then can either use std::move or really use .copy() if a copy is needed.

That’s nice because the compiler finds all problems automatically, but requires to touch quite a lot of places.

willcl-ark commented at 1:39 pm on May 13, 2024: member

Can you explain this? It has a move constructor, …

I think you are correct here, the compiler implicitly generates move-contructors which are enough to fix this issue, if combined with appropriate std::move. I did not think that was the case, but I was using a non-reproducible test. I made my test reproducible and tested:

v27.0
addition of std::move only
addition of std::move + excplicit move constructor(s)
addition of std::move + excplicit move constructor(s) + LD_PRELOAD=mimalloc (just for my own interest)

rest-comparison

The test script requests every block from (0 - 844,000) % 5,000, and measures the RSS after each call.

maflcko commented at 1:46 pm on May 13, 2024: member

either use std::move or really use .copy() if a copy is needed.

Style-wise I found it a bit unfortunate, because it forces code to either specify move or copy explicitly. (There is some beauty to rust, in that it implicitly moves)

whitslack commented at 5:12 pm on May 13, 2024: contributor

I had a PR somewhere where I removed the copy constructor and made it mandatory to call a .copy(). Then you’d get a compile error wherever a copy happens, and then can either use std::move or really use .copy() if a copy is needed.

Don’t do that. You’ll break guaranteed copy elision, which requires the compiler to skip the construction of a temporary object in certain expressions and statements but can only happen if an accessible copy constructor exists (even though it is not called). By deleting the copy constructor, you are denying all opportunities for copy elision and are forcing the compiler always to construct temporaries and to move from them subsequently.

ryanofsky closed this on May 13, 2024

theuni referenced this in commit 0503cbea9a on May 13, 2024

Slow memory leak in v22.0? #24542