💬 maflcko commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1398383803)
```suggestion
DataStream stream{benchmark::data::block413567};
```
(see compile failure)
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1398383803)
```suggestion
DataStream stream{benchmark::data::block413567};
```
(see compile failure)
💬 maflcko commented on pull request "depends: bump libmultiprocess to fix capnproto deprecation warnings":
(https://github.com/bitcoin/bitcoin/pull/28907#issuecomment-1817829246)
lgtm ACK 21bfee0720ba72935d4f9fc4c2a2887ae5b54092
(https://github.com/bitcoin/bitcoin/pull/28907#issuecomment-1817829246)
lgtm ACK 21bfee0720ba72935d4f9fc4c2a2887ae5b54092
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817833689)
Thanks a lot for the log! I am now pretty certain that the allocated blocks from the pool are not counted in `DynamicUsage`. I can reproduce practically exactly same cache sizes as in the log when I set `usage_chunks=0` here (and run 32bit bitcoind on x86): https://github.com/bitcoin/bitcoin/blob/master/src/memusage.h#L201
So the `bucket_count` is calculated correctly which can be seen by the memory jumps.
So either `pool_resource->ChunkSizeBytes()` or `pool_resource->NumAllocatedChunks()`
...
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817833689)
Thanks a lot for the log! I am now pretty certain that the allocated blocks from the pool are not counted in `DynamicUsage`. I can reproduce practically exactly same cache sizes as in the log when I set `usage_chunks=0` here (and run 32bit bitcoind on x86): https://github.com/bitcoin/bitcoin/blob/master/src/memusage.h#L201
So the `bucket_count` is calculated correctly which can be seen by the memory jumps.
So either `pool_resource->ChunkSizeBytes()` or `pool_resource->NumAllocatedChunks()`
...
💬 sipa commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817834734)
@martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817834734)
@martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817837129)
> @martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
Which part of code is responsible for that?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817837129)
> @martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
Which part of code is responsible for that?
💬 sipa commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817838140)
@hebasto The guess for node size is here: https://github.com/bitcoin/bitcoin/blob/v26.0rc2/src/coins.h#L146
Can you try increasing that (say, add 64 to it), and see if that fixes things?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817838140)
@hebasto The guess for node size is here: https://github.com/bitcoin/bitcoin/blob/v26.0rc2/src/coins.h#L146
Can you try increasing that (say, add 64 to it), and see if that fixes things?
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817845209)
> @martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
That sounds like a very likely explanation! Although I'm curious why the estimation would work in x86 32bit but not on ARM
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817845209)
> @martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
That sounds like a very likely explanation! Although I'm curious why the estimation would work in x86 32bit but not on ARM
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817845764)
Might it depend on `max_load_factor`?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817845764)
Might it depend on `max_load_factor`?
💬 sipa commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817848079)
@hebasto No, it shouldn't.
We're essentially trying to guess the `sizeof` of an internal data structure in `std::unordered_map`. If we guess too small, allocations do not go to the pool, but a fallback to the normal allocator is used.
@martinus Should we make sure that even allocations that use the fallback get accounted for in the resource?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817848079)
@hebasto No, it shouldn't.
We're essentially trying to guess the `sizeof` of an internal data structure in `std::unordered_map`. If we guess too small, allocations do not go to the pool, but a fallback to the normal allocator is used.
@martinus Should we make sure that even allocations that use the fallback get accounted for in the resource?
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817850120)
@sipa I thought that too, it would definitely make sense. Although it would need to move the `size_t MallocUsage(size_t alloc);` into the pool too
If the size increase does not help, it could also be that ARM for some reason needs a bigger alignment for its nodes.
In any case it would be interesting to run this again with this commit: https://github.com/martinus/bitcoin/commit/eaa6aaa481c45866815d67dc80c509be15f22974 this will log each time the pool is called with an allocation size it can't
...
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817850120)
@sipa I thought that too, it would definitely make sense. Although it would need to move the `size_t MallocUsage(size_t alloc);` into the pool too
If the size increase does not help, it could also be that ARM for some reason needs a bigger alignment for its nodes.
In any case it would be interesting to run this again with this commit: https://github.com/martinus/bitcoin/commit/eaa6aaa481c45866815d67dc80c509be15f22974 this will log each time the pool is called with an allocation size it can't
...
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817851058)
@martinus
> If the size increase does not help, it could also be that ARM for some reason needs a bigger alignment for its nodes. In any case it would be interesting to run this again with this commit: [martinus@eaa6aaa](https://github.com/martinus/bitcoin/commit/eaa6aaa481c45866815d67dc80c509be15f22974) this will log each time the pool is called with an allocation size it can't handle, which should be very noisy.
I've already tested my own similar commit. There were no log entries for it.
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817851058)
@martinus
> If the size increase does not help, it could also be that ARM for some reason needs a bigger alignment for its nodes. In any case it would be interesting to run this again with this commit: [martinus@eaa6aaa](https://github.com/martinus/bitcoin/commit/eaa6aaa481c45866815d67dc80c509be15f22974) this will log each time the pool is called with an allocation size it can't handle, which should be very noisy.
I've already tested my own similar commit. There were no log entries for it.
💬 sipa commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817851784)
@hebasto Did the `+ 64` change change anything at all?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817851784)
@hebasto Did the `+ 64` change change anything at all?
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817851986)
> @hebasto Did the `+ 64` change change anything at all?
It is running...
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817851986)
> @hebasto Did the `+ 64` change change anything at all?
It is running...
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817852874)
> I've already tested my own similar commit. There were no log entries for it.
There should always be quite a few log lines for the map buckets, even when all works as expected
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817852874)
> I've already tested my own similar commit. There were no log entries for it.
There should always be quite a few log lines for the map buckets, even when all works as expected
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817854534)
> @hebasto Did the `+ 64` change change anything at all?
Tested https://github.com/hebasto/bitcoin/commit/80c02a9fe6607fb0a4abfca66be7be42fd744905.
No behavior change. Still OOM.
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817854534)
> @hebasto Did the `+ 64` change change anything at all?
Tested https://github.com/hebasto/bitcoin/commit/80c02a9fe6607fb0a4abfca66be7be42fd744905.
No behavior change. Still OOM.
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817854605)
> > I've already tested my own similar commit. There were no log entries for it.
>
> There should always be quite a few log lines for the map buckets, even when all works as expected
Testing now...
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817854605)
> > I've already tested my own similar commit. There were no log entries for it.
>
> There should always be quite a few log lines for the map buckets, even when all works as expected
Testing now...
💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1398408939)
Done
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1398408939)
Done
📝 maflcko opened a pull request: " refactor: VectorWriter and SpanReader without nVersion "
(https://github.com/bitcoin/bitcoin/pull/28912)
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.
(https://github.com/bitcoin/bitcoin/pull/28912)
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817869184)
> > I've already tested my own similar commit. There were no log entries for it.
>
> There should always be quite a few log lines for the map buckets, even when all works as expected
The log is flooded with "Allocate: 104 align(8), MAX_BLOCK_SIZE_BYTES=112, ALIGN_BYTES=4"
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817869184)
> > I've already tested my own similar commit. There were no log entries for it.
>
> There should always be quite a few log lines for the map buckets, even when all works as expected
The log is flooded with "Allocate: 104 align(8), MAX_BLOCK_SIZE_BYTES=112, ALIGN_BYTES=4"
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817870240)
> The log is flooded with "Allocate: 104 align(8), MAX_BLOCK_SIZE_BYTES=112, ALIGN_BYTES=4"
Ha! This is the issue right there. For whatever reason the nodes require alignment of 8 bytes, but the pool is configured to use the platform's 4 byte instead. Thanks a lot for all the testing! I'll try to prepare a fix
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817870240)
> The log is flooded with "Allocate: 104 align(8), MAX_BLOCK_SIZE_BYTES=112, ALIGN_BYTES=4"
Ha! This is the issue right there. For whatever reason the nodes require alignment of 8 bytes, but the pool is configured to use the platform's 4 byte instead. Thanks a lot for all the testing! I'll try to prepare a fix