💬 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
💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382215744)
Mark constructors as `explicit`.
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382215744)
Mark constructors as `explicit`.
💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382220405)
In commit 80ea259b0b8bb08637d1cb75f65ec342d03eae7a, I find the second sentence of the commit message confusing. Is there something missing?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382220405)
In commit 80ea259b0b8bb08637d1cb75f65ec342d03eae7a, I find the second sentence of the commit message confusing. Is there something missing?
💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1398410709)
The bool in `removeTx` is always `false` now, maybe remove it entirely?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1398410709)
The bool in `removeTx` is always `false` now, maybe remove it entirely?
💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1398414563)
Nit: Remove the unused argument names in the interface functions?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1398414563)
Nit: Remove the unused argument names in the interface functions?
💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1398406098)
Nit: Doesn't really matter here, but could use list initialization.
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1398406098)
Nit: Doesn't really matter here, but could use list initialization.
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817880300)
TIL that on ARM 32bit an `int64_t` has 8 byte alignment, even when a pointer has 4 byte alignment: https://godbolt.org/z/xW3avfGef
@hebasto could you please try this commit, I believe this should fix the issue: https://github.com/martinus/bitcoin/commit/72918e0d80455ece4aa96228be92246a83c8ad50
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817880300)
TIL that on ARM 32bit an `int64_t` has 8 byte alignment, even when a pointer has 4 byte alignment: https://godbolt.org/z/xW3avfGef
@hebasto could you please try this commit, I believe this should fix the issue: https://github.com/martinus/bitcoin/commit/72918e0d80455ece4aa96228be92246a83c8ad50