--- Log opened Wed Mar 08 00:00:05 2023
00:02 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 260 seconds]
00:04 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
00:08 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 248 seconds]
00:31 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
00:36 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 264 seconds]
00:37 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
00:42 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 264 seconds]
00:48 -!- __gotcha [~Thunderbi@213.181.59.198] has joined #bitcoin-core-pr-reviews
00:59 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
01:04 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 265 seconds]
01:05 -!- Pasha [~Cory@068-187-125-109.res.spectrum.com] has quit [Ping timeout: 255 seconds]
01:05 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
01:06 -!- Cory [~Cory@068-187-125-109.res.spectrum.com] has joined #bitcoin-core-pr-reviews
01:09 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 255 seconds]
01:26 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
01:27 -!- koolazer [~koo@user/koolazer] has quit [Read error: Connection reset by peer]
01:30 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 265 seconds]
01:34 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
01:39 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 246 seconds]
01:43 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
01:49 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 256 seconds]
01:55 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
02:00 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 248 seconds]
02:13 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
02:39 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Remote host closed the connection]
02:39 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
02:44 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 260 seconds]
02:50 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
02:51 -!- chipxxx [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has joined #bitcoin-core-pr-reviews
02:53 -!- chip__ [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has quit [Ping timeout: 256 seconds]
02:55 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 260 seconds]
02:56 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
03:01 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 265 seconds]
03:10 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
03:44 -!- __gotcha [~Thunderbi@213.181.59.198] has quit [Ping timeout: 255 seconds]
03:56 -!- __gotcha [~Thunderbi@213.181.59.198] has joined #bitcoin-core-pr-reviews
04:00 -!- __gotcha [~Thunderbi@213.181.59.198] has quit [Ping timeout: 248 seconds]
04:12 -!- martinus [~martinus@212095005106.public.telering.at] has joined #bitcoin-core-pr-reviews
04:13 -!- b_101_ [~robert@193.37.254.131] has joined #bitcoin-core-pr-reviews
04:14 -!- b_101 [~robert@193.37.254.131] has quit [Ping timeout: 260 seconds]
04:43 -!- __gotcha [~Thunderbi@213.181.59.198] has joined #bitcoin-core-pr-reviews
06:31 -!- karliatto [~karliatto@cst-prg-24-220.cust.vodafone.cz] has joined #bitcoin-core-pr-reviews
06:31 -!- karliatto_ [~karliatto@cst-prg-24-220.cust.vodafone.cz] has joined #bitcoin-core-pr-reviews
07:12 -!- chip__ [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has joined #bitcoin-core-pr-reviews
07:14 -!- chipxxx [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has quit [Ping timeout: 248 seconds]
08:15 -!- abubakar [~abubakars@197.210.53.253] has joined #bitcoin-core-pr-reviews
08:16 -!- chip__ [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has quit [Remote host closed the connection]
08:26 -!- pakaro [~quassel@104.221.123.79] has joined #bitcoin-core-pr-reviews
08:26 -!- pablomartin [~pablomart@185.174.108.104] has joined #bitcoin-core-pr-reviews
08:31 < LarryRuane> we'll get started here in about 30 minutes https://bitcoincore.reviews/25325
08:37 -!- Zenton [~user@user/zenton] has quit [Ping timeout: 255 seconds]
08:39 -!- __gotcha [~Thunderbi@213.181.59.198] has quit [Ping timeout: 248 seconds]
08:40 -!- DaveBeer [~DaveBeer@ip-62-245-124-60.bb.vodafone.cz] has joined #bitcoin-core-pr-reviews
08:42 -!- karliatto__ [~karliatto@cst-prg-24-220.cust.vodafone.cz] has joined #bitcoin-core-pr-reviews
08:42 -!- karliatto_ [~karliatto@cst-prg-24-220.cust.vodafone.cz] has quit [Read error: Connection reset by peer]
08:43 -!- 029AANYXB [~lowhope@cow9.org] has quit [Ping timeout: 268 seconds]
08:44 -!- lowhope_ [~lowhope@cow9.org] has joined #bitcoin-core-pr-reviews
08:56 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
08:56 -!- abubakar [~abubakars@197.210.53.253] has quit [Read error: Connection reset by peer]
08:56 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
08:57 -!- abubakar [~abubakars@197.210.77.7] has joined #bitcoin-core-pr-reviews
08:59 -!- hernanmarino [~hernanmar@181.99.169.107] has joined #bitcoin-core-pr-reviews
08:59 -!- codo [~codeautis@user/codeautist] has joined #bitcoin-core-pr-reviews
09:00 < LarryRuane> #startmeeting
09:00 < pablomartin> hello all!
09:00 < effexzi> Hello every1
09:00 < glozow> hi
09:00 < DaveBeer> hi
09:00 < martinus> hi!
09:00 < abubakar> hi
09:00 < hernanmarino> Helloooo
09:00 < svav> Hi
09:00 < LarryRuane> hi everyone, please say hi so we know who's here!
09:00 < glozow> hi martinus, great to have you here!
09:00 < codo> hi
09:00 < LarryRuane> yes, thank you for being here, @martinus !
09:00 < LarryRuane> any first-time attendees here today?
09:00 < jonatack1> hi
09:00 < martinus> thanks, and thanks LarryRuane for having my PR here!
09:01 < DaveBeer> yes, I'm here for first time
09:01 -!- jonatack1 [~jonatack@user/jonatack] has quit [Quit: WeeChat 3.8]
09:01 < LarryRuane> here's the write-up for today's review club session: https://bitcoincore.reviews/25325
09:01 -!- pakaro [~quassel@104.221.123.79] has quit [Ping timeout: 252 seconds]
09:02 -!- pakaro [~quassel@142.243.254.224] has joined #bitcoin-core-pr-reviews
09:02 < LarryRuane> welcome, @DaveBeer ! Feel free to ask away, any questions, even if we've moved on, continuing with previous threads is fine!
09:02 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
09:02 < pakaro> hi
09:03 < LarryRuane> first question, our usual: Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
09:03 < DaveBeer> y, read notes, briefly looked at code
09:03 -!- pakaro2 [~pakaro2@142.243.254.224] has joined #bitcoin-core-pr-reviews
09:03 < abubakar> Yes, Approach ACK, read through the PR commits and have an understanding of the PR code
09:03 < LarryRuane> Also, does anyone have any questions on the Notes (intended to be background material)? We can discuss those first if there are any
09:03 < hernanmarino> 70% yes
09:04 -!- Steve68 [~Steve@halo.irb-cisr.gc.ca] has joined #bitcoin-core-pr-reviews
09:04 < martinus> I'd ACK it because I wrote it ;)
09:04 < pakaro> I was wondering where the data, if any, for cache hit rate is
09:04 < LarryRuane> *whew* +1 @martinus 
09:04 < pakaro> ie. how often we access utxo from cache and how often we reach to disk
09:05 < LarryRuane> that's a good question, I don't know if there's any tracking of that, does anyone know?
09:05 < pablomartin> yes, read pr, notes, answered some questions, pending running some benchs & testing before commenting on the pr
09:06 -!- pakaro2 [~pakaro2@142.243.254.224] has quit [Client Quit]
09:06 < jonatack> Reviewed both the original PR (https://github.com/bitcoin/bitcoin/pull/22702) and then this newer one. Have also been running them for a long time on my nodes.
09:06 < Steve68> Hello, this is my first time here, I may be a little lost. Is there a Bitcoin core meeting on wednesdays?
09:07 < LarryRuane> I was looking over the notes just before the meeting, and even though there are lots of notes, there may be a few concepts that people aren't too familiar with... maybe we can take a couple minutes to cover some basics...
09:07 < LarryRuane> were most of you aware that C++ standard library contains allow you to use a custom memory allocator?
09:07 < hernanmarino> jonatack: great, this deserves a long testing 
09:07 < LarryRuane> for example, take a look at the `Allocator` template argument here https://en.cppreference.com/w/cpp/container/unordered_map
09:07 < jonatack> Steve68: https://github.com/jonatack/bitcoin-development/blob/master/bitcoin-core-dev-irc-meetings.txt
09:08 < LarryRuane> the default memory allocator is the standard system allocator (which basically just does `new` and `delete`), but you can override the default
09:09 < Steve68> thank you!
09:09 < LarryRuane> (note that in c++ if you override a template argument, you must specify all preceding template arguments, even if you just want the defaults for those)
09:09 < LarryRuane> when a container object (like an `std::unordered_map` instance) wants to allocate memory, it uses its configured allocator (standard or custom)
09:10 < LarryRuane> (@martinus correct me if I'm wrong on any of this!)
09:10 < LarryRuane> this PR changes the allocator for the `unordered_map` used in the coins cache, and _only_ that one (not all unordered maps)
09:10 < martinus> all good from my side :)
09:11 < LarryRuane> it changes it to one implemented by this PR (so the PR has two main parts, implementing a new allocator, and making this one particular unordered_map use it)
09:11 < LarryRuane> Any of that unclear, or want to discuss?
09:12 -!- Zenton [~user@user/zenton] has joined #bitcoin-core-pr-reviews
09:12 < pakaro> very well described
09:12 -!- abubakar [~abubakars@197.210.77.7] has quit [Read error: Connection reset by peer]
09:12 -!- b_101 [~robert@189.236.18.183] has joined #bitcoin-core-pr-reviews
09:12 -!- abubakar [~abubakars@197.210.76.171] has joined #bitcoin-core-pr-reviews
09:13 < jonatack> for info, martinus has a blog at https://martin.ankerl.com and wrote the benchmarking library that we use since a couple of years now in bitcoin core
09:13 < LarryRuane> Okay we can get into the questions then ... one question I forgot to ask, why is there no mutex locking in this PR?
09:13 < martinus> Maybe let me add the main reasons for this custom allocator: unordered_map does one allocation per entry, and this can be costly: malloc/free can be relatively slow, and it also has a memory overhead. This PR makes it faster and reduces that overhead.
09:13 -!- b_101_ [~robert@193.37.254.131] has quit [Ping timeout: 255 seconds]
09:13 < LarryRuane> jonatack: TIL -- thanks for the link!!
09:14 < jonatack> (on the blog you can also see his extensive work on hashmap data structures in C++)
09:14 < LarryRuane> is that like standard c++ (not just bitcoin core)?
09:15 < martinus> It's per the standard that std::unordered_map has to perform at least one allocation per node (entry), and there's no way around this. Other hashmap implementations do this differently so they can be faster
09:16 < martinus> But changing the whole hashmap of such an integral part can be dangerous (hashmaps are hard to get right), so this PR just changes the allocator to get most of the benefits
09:17 < LarryRuane> martinus: thanks, and you mentioned that malloc/free (the lowest-level dynamic memory allocation primitives) can be slow, this is question 7 (ok if we go out of order!) ... can you give us reasons why?
09:18 < pablomartin> perhaps it's worth mentioning that originally, the focus was on the hashmap (to make std::unordered_map faster...) - https://github.com/bitcoin/bitcoin/pull/16718 - by jamesob
09:19 < martinus> sure, I mean malloc is really well optimized, but it has to be generically implemented for whatever size whatever thread currently wants to allocate. But with the unordered_map we know that we need a lot of nodes, and we know that each of them has exactly the same size; so we can use that to optimize for this specific case.
09:19 < abubakar> The Allocator implementation avoids memory fragmentation. This ensures no gaps between allocated blocks of memory, also avoids the overhead of calling malloc() and free() for every allocation and deallocation. Instead, the allocator manages its own pool of memory and allocates and deallocates blocks from this pool as needed. reduces the number of system calls required for memory 
09:19 < abubakar> allocation and deallocation
09:20 -!- Amirreza [~Amirreza7@89.219.97.60] has joined #bitcoin-core-pr-reviews
09:21 < LarryRuane> martinus: abubakar: yes, thanks ... I'm not sure about reducing the number of system calls, though... aren't most malloc and free just within the runtime library?
09:21 -!- Zenton [~user@user/zenton] has quit [Ping timeout: 268 seconds]
09:22 < DaveBeer> yes they manage some kind of tree like structure for memory chunks, as long as large enough chunk is available, system call should not be needed
09:22 < DaveBeer> in runtime library that is
09:22 < LarryRuane> what I was thinking about the performance improvement was that no mutex locking is required ... i mean, there may be locking at a higher level (around the entire map, if multiple threads can access it), but not for each individual memory alloc and dealloc
09:24 < abubakar> LarryRuane: yes thanks.
09:24 < LarryRuane> and for question 8, "Why does this allocator use less memory (for the coins cache) than the standard allocator?", yes, I think @abubakar hit on the main reason, all the allocations are tightly packed together ... as opposed to individual allocations, where there's some (hidden to us) overhead
09:25 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
09:25 < LarryRuane> so I think martinus answered question 2, the goal of this PR ... shall we go to question 3?
09:25 < jonatack> LarryRuane: re locking: that said, my understanding is that locking itself isn't usually expensive, but lock contention is
09:26 -!- b_101 [~robert@189.236.18.183] has quit [Ping timeout: 268 seconds]
09:26 -!- Amirreza [~Amirreza7@89.219.97.60] has quit [Remote host closed the connection]
09:26 -!- b_101 [~robert@193.37.254.131] has joined #bitcoin-core-pr-reviews
09:26 < LarryRuane> yes, and there would be a lot of contention for memory allocation and deallocation, is that right?
09:27 < jonatack> I don't know offhand
09:27 < jonatack> just making the distinction
09:27 < LarryRuane> i know there's a build that measures lock contention (`--enable-debug` i think), but i don't know if it measures dynamic memory lock contention
09:28 < LarryRuane> Here's question 3, might be kind of easy: Why is CCoinsMap implemented as an std::unordered_map used instead of a std::map? What are the advantages of each?
09:28 < hernanmarino> it's faster 
09:28 < abubakar> for question 3.. Because Coins are not ordered, we want fast read time, std::unordered_map uses hash table while std::map uses BST. 
09:28 < hernanmarino> unordered_map, I mean
09:29 < abubakar> Std::unordered is good to access element fast that dont care about order, whereas std::map. Is good for sorted entries.
09:29 < DaveBeer> unordered has O(1) access time while regular map has O(log(N)) access time and Coins don't need to be visited in any particular order, so unordered_map is enough
09:29 < pakaro> there's no need for order, since each utxo in the utxo is unique. +1 davebeer abubakar
09:30 < jonatack> yes, DEBUG_LOCKCONTENTION is now built into the debug build and you can enable the lock logging category to see it
09:30 < jonatack> see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#debug_lockcontention for more
09:30 < LarryRuane> hernanmarino: abubakar: DaveBeer: pakaro: +1 - good answers, those are my understandings too
09:31 < LarryRuane> i think i've noticed quite a few maps that could be unordered_maps ... but if they're not performance-critical, i guess it doesn't matter
09:31 < pablomartin> faster lookup, customizable hashing, faster insertion & removal, less memory usage...
09:31 < LarryRuane> i think someone said that when bitcoin core was first written, there was no `std::unordered_map` yet, only added later ... does anyone know if that's true?
09:32 < LarryRuane> so (if that's true), some are just leftover from the early days
09:32 < martinus> When maps are very small (say 20 elements or so) sometimes std::map can be faster, but in this case the CCoinMap is really huge so std::unordered_map is definitely faster
09:32 < DaveBeer> unordered_map was introduced only in C++11 I think
09:32 < hernanmarino> LarryRuane, I believe it s true 
09:32 < hernanmarino> Davebeer: +1
09:32 < jonatack> (but our lock contention logging is only related to the locks we call in src/sync.h)
09:32 -!- Zenton [~user@user/zenton] has joined #bitcoin-core-pr-reviews
09:33 < LarryRuane> jonatack: i see, not standard library locks
09:33 < hernanmarino> Also, the custom allocators where used in this PR where introduced in C++17
09:33 < hernanmarino> that*
09:33 < LarryRuane> continue discussing but let me put question 6 out there... The PoolResource takes one argument, chunk_size_bytes. What does this argument do, and what are the tradeoffs when deciding on this value?
09:35 < martinus> hernanmarino: I think the std::allocators were introduced in C++11, in C++17 the pmr allocators were introduced. But they are still not implemented everywhere...
09:35 < hernanmarino> martinus: thanks for clarifying
09:35 < pakaro> the number of bytes to give to each request for memory-space?
09:35 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Remote host closed the connection]
09:36 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
09:36 < LarryRuane> martinus: oh that's actually something i was wondering, the PR description says "A memory resource similar to std::pmr::unsynchronized_pool_resource, but optimized for node-based containers" ... I'm unsure if `pmr` could have been used, but would not have been as efficient? Or not possible?
09:36 < pakaro> if it's too large, we are wasting memory, if it is too small, an entry would not fit, and i assume the program would crash...this is why we do the *4 iirc...i may be off here
09:37 < jonatack> Bitcoin Core began with C++98, so yes, unordered_map didn't arrive until later IIRC
09:38 < LarryRuane> pakaro: +1 on the first part, there's always some unused space in the most recent chunk ... but if it's too small, the PoolAllocator just does a regular new (and later delete)
09:38 < LarryRuane> won't crash
09:38 < martinus> LarryRuane: Actually I first implemented this PR by using the pmr allocators and that way it was quite a bit simpler, but unfortunately that did not compile everywhere because libc++ still doesn't implement it. I think on MacOS that didn't work. It worked locally though with my libstdc++
09:39 < LarryRuane> oh i see... that must have been a frustrating day, if you had it all working on your machine! 
09:40 < LarryRuane> so on the question of choosing the chunk size, if it's too large there's wasted memory, and if it's too small, then there are more system allocations (which we know are somewhat slow)
09:42 < LarryRuane> martinus: I was wondering, did you consider making `chunk_size_bytes` a template argument? What were the considerations there?
09:43 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Remote host closed the connection]
09:44 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
09:44 < martinus> LarryRuane: yes I thought about making it either a template or an argument in the constructor, but then I though I should try to keep the PR as simple as possible. It's already complex enough and requires quite a lot of knowledge of internals. It might be a future extension if the allocator is used more widely
09:44 < LarryRuane> I guess also, the more stuff you template, the more actual code memory usage there can be!
09:45 < LarryRuane> since each combination of template values creates a whole new instance of all the code (IIUC)
09:46 < LarryRuane> (but probably code memory usage is not important)
09:47 < martinus> Also, allocating a big chunk of memory is actually quite cheap, at least in Linux this is done lazily: the malloc of the chunk doesn't really do much, only when the memory is actually used (read / write) the operating system goes and makes that page available.
09:47 < Murch> Is that why you needed the two different variants of new?
09:47 < LarryRuane> As long as you mentioned "if the allocator is used more widely" -- question 10 is: Are there other data structures that can take advantage of this new allocator?
09:48 < Murch> To make sure it actually is made available?
09:48 < martinus> Murch: no this is completely hidden and done from the kernel as far as I know, this has nothing to do with the different versions of new
09:48 < Murch> Oh okay
09:49 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 268 seconds]
09:49 < LarryRuane> yes, there's the operator new: https://en.cppreference.com/w/cpp/memory/new/operator_new and the expression new: https://en.cppreference.com/w/cpp/language/new ... which i never knew about before!
09:49 < Murch> TBH, this all seems like higher arcane mysteries to me ^^
09:49 < LarryRuane> It's like the expression new can be given _already allocated_ memory, right? (they call it "placement-params")
09:50 < hernanmarino> LarryRuane: On q. 10 Mempool is the first that comes to mind , but i don't think it can benefit a lot from this... perhaps something more heavily used during IBD? Blockindexes ?? I am not sure if they really use a lot of memory ...
09:50 < LarryRuane> yes, the uses of `new` in this PR were unfamiliar
09:50 < jonatack> Murch: once I tweeted about enjoying C++, and a long-time Ruby friend replied "Jon, are you OK? Blink twice if under duress" :)
09:51 < martinus> ah yes, allocating with operator new and in-place construction with new(...) are completely different things, but they have the same name... It's C++ being as obfuscated as it can be
09:51 < LarryRuane> hernanmarino: +1 - I was thinking mempool also, but I'm unsure
09:51 < hernanmarino> I'm just guessing, didn thoroughly reviewd the code to answer this one 
09:51 < Murch> jonatack: Yeah, I’m definitely still not in it because of C++ 😅
09:51 < hernanmarino> :)
09:52 < Murch> 😀😆😀😆
09:52 < LarryRuane> martinus: kudos for figuring out some of this very weird (but needed) c++!
09:53 < Murch> hernanmarino: I’m not sure. A lot of transactions have pretty similar sizes, and the big ones could just be allocated separately. Might be useful to cover the ~24% one input one output txs, and the 46% one input two output txs
09:54 < LarryRuane> I think this is a great PR, I really want it to merge ... it was really interesting to read about people's concerns with safety (i think that's why the earlier attempt didn't happen)
09:54 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
09:55 < LarryRuane> Okay only 5 minutes left, anyone want to take on question 9: Can you think of a usage pattern (perhaps outside of the coins cache) that might cause this allocator to use more memory than the standard allocator?
09:55 < LarryRuane> Also, if anyone would like to throw out any answers to the sub-questions in question 5... I think those are interesting c++ questions
09:56 < hernanmarino> Murch: Agree. I was also thinking about something more heavily used during IBD, and perhaps it's not the case with the mempool
09:56 < pakaro> my understanding of static_assert is that it will throw a compilation error in debug mode, when an 'assert' will not throw an error in debug mode.
09:57 < pakaro> re. q5
09:57 < LarryRuane> pakaro: no actually, assert crashes the node even in a non-debug build
09:58 < pakaro> then im not sure of the difference between static_assert & assert
09:58 < hernanmarino> re q9 I'm just guessing, but perhaps allocating only a few big chunks of memory, instead of several small ones ...
09:58 < LarryRuane> it's interesting to look at *where* those `static_assert`s are ... they're not inside functions! (for fun I tried changing one of them to a regular assert, and it wouldn't compile)
09:58 < abubakar>  pre allocated memory more than necessary might result to alot of unused memory
09:58 < jonatack> pakaro: static assert is checked at compilation time
09:58 < jonatack> vs runtime for assert
09:58 -!- karliatto__ [~karliatto@cst-prg-24-220.cust.vodafone.cz] has quit [Ping timeout: 248 seconds]
09:58 -!- karliatto [~karliatto@cst-prg-24-220.cust.vodafone.cz] has quit [Ping timeout: 248 seconds]
09:58 < pablomartin> q9: perhaps in scenarios where the data structure contains a mixture of large and small objects
09:58 < LarryRuane> yes, static_assert triggers for any kind of build
09:59 -!- pakaro [~quassel@142.243.254.224] has quit [Remote host closed the connection]
09:59 < LarryRuane> pablomartin: yes, that's what i was thinking ... like if you allocate a bunch of one size of objects, then free them all ... then allocate a bunch of other sizes but never that first size, then they're sort of stuck in their freelist
10:00 < LarryRuane> ok we're at time, thanks to all of you!! and especially @martinus ! Please go review the PR!
10:00 < martinus> static_asserts are great because it immediately gives you a compile error. So if it compiles you can be sure that all static_assert's are correct. assert() are only evaluated when that code is actually run, so ideally use static_assert when possible
10:00 < abubakar> final ensures that the PoolResource class can be modified outside it's scope
10:00 < pablomartin> thanks all!
10:00 < LarryRuane> #endmeeting
10:00 < effexzi> Thanks every1
10:00 < martinus> Thanks a lot LarryRuane for moderating this meeting!
10:00 < abubakar> LarryRuane: thanks for hosting
10:01 < Murch> Thanks Larry
10:01 < hernanmarino> thanks Larry for hosting, and martinus for great improvement
10:01 < LarryRuane> yes, and a regular assert might not be hit ... until post-release, on user machines!
10:01 < Murch> Also thanks to martinus for joining and giving more details!
10:01 -!- pakaro [~quassel@142.243.254.224] has joined #bitcoin-core-pr-reviews
10:02 < DaveBeer> thanks Larry and martinus
10:02 < pakaro> computer crashed. assuming end has been called. thanks everyone!
10:02 < LarryRuane> pakaro: yes, thanks for being here!
10:02 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 248 seconds]
10:03 < jonatack> LarryRuane: (ALIGN_BYTES & (ALIGN_BYTES - 1)) == 0 tests for being a multiple of two IIRC, and the reason to use it over modulo could be performance. If I'm not confused, I reviewed it a while ago.
10:03 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
10:03 < LarryRuane> abubakar: I think of `final` as meaning that you can't create a class that uses this one as its base class ... but I'm not really sure if there's any special reason it's used here
10:03 -!- Amirreza [~Amirreza7@89.219.97.60] has joined #bitcoin-core-pr-reviews
10:04 < Murch> jonatack: Yeah, that was pretty nifty
10:04 < jonatack> (IIRC modulo is a fairly slow operation)
10:04 < Amirreza> Hi everyone
10:04 < pakaro> +1 murch very nifty
10:04 < LarryRuane> jonatack: yes that's correct ... only a power of 2, or zero, will make that expression zero
10:04 < LarryRuane> but you can't even use modulo, right?
10:05 < jonatack> LarryRuane: was referring to the comment at https://github.com/bitcoin/bitcoin/pull/25325/files#r1124914676
10:05 < Murch> Amirreza: Welcome, however the Bitcoin Core PR Review Club just finished
10:05 < LarryRuane> you essentially want to check of just 1 bit is set ... there are some instruction sets that have a "popcount" (number of bits set) instruction, but obviously that's pretty low-level
10:05 < jonatack> I recall messing around with adding static asserts with modulo as you suggested and checking
10:06 < martinus> yeah the "final" here is not really necessary, it's just that no one can use it as a base class. Actually I'm not sure why I have it here, but it doesn't hurt
10:06 < Amirreza> Murch, Oh, I'm too late. I just did a mistake in timing!
10:06 < LarryRuane> martinus: ok cool ... is there any special reason for the `[[nodiscard]]`? I think it's nice, but any special reason?
10:07 < jonatack> LarryRuane: nodiscard is good to add for getters to ensure that the result is in fact used
10:08 < jonatack> LarryRuane: TIL wrt to your popcount comment, thanks!
10:08 < LarryRuane> ok, makes sense, because it's a pure function i think it's called (no side-effects), so if you're not using the result why are you calling it??
10:08 < martinus> I usually use [[nodiscard]] everywhere, because when I return something I usually want that to be used; and if one doesn't use the return value it usually means that someone called the method but didn't have to. Especially all const methods should be [[nodiscard]]
10:09 < LarryRuane> martinus: i see, good to know... almost seems like `const` functions can be nodiscard by default?
10:09 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
10:09 < martinus> LarryRuane: yes I think it would be better to have that as default, but It's C++ so it won't change :)
10:09 < jonatack> martinus: good points
10:10 < LarryRuane> but it is interesting i guess that even `const` methods can have side-effects -- the `mutable` variables ... but callers shouldn't know about those side-effects
10:10 < LarryRuane> martinus: thanks.. I think your code is really good for all of us to use as a model!
10:10 -!- Amirreza [~Amirreza7@89.219.97.60] has quit [Quit: Leaving]
10:11 < martinus> thanks!
10:12 -!- pablomartin [~pablomart@185.174.108.104] has quit [Ping timeout: 246 seconds]
10:12 < jonatack> Thanks for your long-standing work and patience to improve the performance here martinus, and thank you LarryRuane for choosing to review this pull and host a review club about it.
10:13 < pakaro> +1 jonatack - reading the comments from 2019 puts in perspective the amount of deliberation that goes into a pr like this
10:13 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 246 seconds]
10:13 < DaveBeer> yeah this one was hard for me to parse '=D    void* storage = ::operator new (m_chunk_size_bytes, std::align_val_t{ELEM_ALIGN_BYTES});
10:13 < DaveBeer>         m_available_memory_it = new (storage) std::byte[m_chunk_size_bytes];
10:14 < LarryRuane> jonatack: +1 about martinus, and you're very welcome! I really enjoyed learning about this one!
10:15 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
10:15 < pakaro> +larryruane i learnt a lot from your notes and comments in the pr
10:16 < LarryRuane> DaveBeer: yes me too, but i think it's *almost* the same as a normal new, like `void* storage = new std::bytes[m_chunk_size_bytes]` except that we want it aligned (the default new with a one-byte size wouldn't be aligned by 8 bytes)
10:16 < abubakar> same really good notes Larry :)
10:16 -!- Steve68 [~Steve@halo.irb-cisr.gc.ca] has quit [Ping timeout: 255 seconds]
10:16 < LarryRuane> pakaro: abubakar: thanks
10:18 < jonatack> +1 excellent notes. Oh and whoever PRs the meeting log could maybe also drop the word "used" from Question 3.
10:19 < martinus> right, I want it to be aligned correctly. Also note that I'm always using ::operator new (the initial ::), because otherwise it could be that we accidentally use an overloaded version of operator new
10:19 < DaveBeer> LarryRuane: I see, thanks, I wasn't even aware of the new expression until today
10:19 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
10:20 < LarryRuane> oh good catch @jonatack yes I'll fix that!
10:20 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
10:21 < LarryRuane> martinus: oh that's really interesting, i thought it was only so you could specify the alignment ... can you specify alignment without the `::operator`?
10:22 < DaveBeer> * new expression with placement params
10:22 -!- Steve42 [~Steve@halo.irb-cisr.gc.ca] has joined #bitcoin-core-pr-reviews
10:23 < LarryRuane> DaveBeer: i wasn't either, until a few days ago!
10:25 < martinus> You can use  void* storage = operator new (m_chunk_size_bytes, std::align_val_t{ELEM_ALIGN_BYTES}); just fine,  but then you risk of using some other operator new. It's safer to always use ::operator new, unless you really want other operator new to work. So far I never needed that...
10:25 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 252 seconds]
10:27 < LarryRuane> I see.. this reminds me of something else I've been wordering (sorry to keep you!) ... I see quite a few references to `::cs_main` but also the older just `cs_main` ... is that for a similar reason, just to be sure we get the global one? even though there's no other `cs_main` in the codebase?
10:28 < martinus> I don't know about cs_main, but probably; ::cs_main will always use the one in global namespace. Maybe tests have their own cs_main in a different namespace, then they can't use ::cs_main
10:28 < LarryRuane> maybe, yes, that makes sense
10:29 < LarryRuane> doesn't seem consistent, some new PRs use `::cs_main` but not all of even the new code
10:29 -!- DaveBeer [~DaveBeer@ip-62-245-124-60.bb.vodafone.cz] has quit [Quit: Connection closed]
10:30 -!- chip_x [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has joined #bitcoin-core-pr-reviews
10:31 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
10:35 -!- abubakar [~abubakars@197.210.76.171] has quit [Read error: Connection reset by peer]
10:35 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
10:37 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
10:42 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 246 seconds]
10:48 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
10:49 -!- pakaro [~quassel@142.243.254.224] has quit [Ping timeout: 248 seconds]
10:53 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 264 seconds]
10:54 -!- codo [~codeautis@user/codeautist] has left #bitcoin-core-pr-reviews [nil]
10:54 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
10:58 -!- TheCharlatan [~drgrid@2a01:4f9:4a:2adc::2] has joined #bitcoin-core-pr-reviews
10:59 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 246 seconds]
11:01 -!- pakaro [~quassel@142.243.254.224] has joined #bitcoin-core-pr-reviews
11:03 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
11:07 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 252 seconds]
11:09 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
11:11 -!- pakaro [~quassel@142.243.254.224] has quit [Ping timeout: 248 seconds]
11:30 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 255 seconds]
11:31 -!- Steve42 [~Steve@halo.irb-cisr.gc.ca] has quit [Quit: Connection closed]
11:31 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
11:31 < jonatack> LarryRuane: I was disconnected, but in case no one replied: About ::cs_main vs cs_main, it's currently preferred to use the global namespace reference (::cs_main). The cs_main calls tend to be older ones. 
11:37 -!- chip__ [~chip@bl20-128-149.dsl.telepac.pt] has joined #bitcoin-core-pr-reviews
11:39 -!- chip_x [~chip@2001:8a0:f620:6500:8f6d:c640:c292:171c] has quit [Ping timeout: 248 seconds]
11:50 -!- b_101 [~robert@193.37.254.131] has quit [Ping timeout: 248 seconds]
11:51 -!- b_101 [~robert@185.242.5.35] has joined #bitcoin-core-pr-reviews
11:51 -!- pablomartin [~pablomart@185.174.108.104] has joined #bitcoin-core-pr-reviews
12:03 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:11 -!- pablomartin [~pablomart@185.174.108.104] has quit [Remote host closed the connection]
12:11 -!- pablomartin [~pablomart@185.174.108.104] has joined #bitcoin-core-pr-reviews
12:12 -!- hernanmarino [~hernanmar@181.99.169.107] has quit [Quit: Leaving]
12:15 -!- pablomartin [~pablomart@185.174.108.104] has quit [Remote host closed the connection]
12:15 -!- pablomartin [~pablomart@185.174.108.104] has joined #bitcoin-core-pr-reviews
12:36 -!- pablomartin [~pablomart@185.174.108.104] has quit [Ping timeout: 276 seconds]
12:56 -!- chip_x [~chip@bl20-128-149.dsl.telepac.pt] has joined #bitcoin-core-pr-reviews
12:58 -!- chip__ [~chip@bl20-128-149.dsl.telepac.pt] has quit [Ping timeout: 276 seconds]
13:16 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
13:38 -!- karliatto [~karliatto@cst-prg-24-220.cust.vodafone.cz] has joined #bitcoin-core-pr-reviews
13:38 -!- karliatto_ [~karliatto@cst-prg-24-220.cust.vodafone.cz] has joined #bitcoin-core-pr-reviews
14:08 -!- karliatto [~karliatto@cst-prg-24-220.cust.vodafone.cz] has quit [Remote host closed the connection]
14:08 -!- karliatto_ [~karliatto@cst-prg-24-220.cust.vodafone.cz] has quit [Remote host closed the connection]
14:22 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Remote host closed the connection]
14:38 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
14:43 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
15:11 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
15:16 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
15:30 < LarryRuane> jonatack: thanks!
15:44 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
15:49 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
16:21 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
16:26 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
16:54 -!- greypw25460021 [~greypw254@grey.pw] has joined #bitcoin-core-pr-reviews
16:55 -!- jamesob6 [~jamesob@141.156.173.67] has joined #bitcoin-core-pr-reviews
16:56 -!- Pasha [~Cory@068-187-125-109.res.spectrum.com] has joined #bitcoin-core-pr-reviews
16:56 -!- Cory [~Cory@068-187-125-109.res.spectrum.com] has quit [Ping timeout: 255 seconds]
16:56 -!- greypw2546002 [~greypw254@grey.pw] has quit [Ping timeout: 255 seconds]
16:56 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
16:56 -!- jamesob [~jamesob@141.156.173.67] has quit [Ping timeout: 255 seconds]
16:56 -!- jamesob6 is now known as jamesob
17:00 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
17:30 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
17:34 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 260 seconds]
18:04 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
18:09 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
18:37 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
18:41 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 255 seconds]
19:09 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
19:13 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 248 seconds]
19:27 -!- Livestradamus [~Livestrad@user/livestradamus] has quit [Quit: ~Peace~]
19:42 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
19:46 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 260 seconds]
20:18 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
20:22 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
20:45 -!- grettke [~grettke@184.55.131.155] has quit [Quit: grettke]
20:53 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
20:58 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
21:27 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
21:32 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 255 seconds]
22:01 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
22:06 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
22:35 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
22:39 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
23:11 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
23:16 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
23:45 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has joined #bitcoin-core-pr-reviews
23:50 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:90e0:e540:37e9:69bb] has quit [Ping timeout: 248 seconds]
--- Log closed Thu Mar 09 00:00:06 2023