16:57:28 Meeting in a bit more than 1 hour 18:00:02 Meeting time. Hello! https://github.com/monero-project/meta/issues/1297 18:00:51 hello 18:01:09 *waves* 18:01:10 hello 18:02:36 Ok, what are the reports from last week? 18:03:02 SNeedlewoods: I think a 2 round PR makes sense like you have it for the commits (add functions to wallet API first, then separate PR's for using that new API exclusively for the CLI and another for RPC) 18:04:00 the latter PR's could include the commit from the first PR in the history, and build off it with a new separate commit 18:05:54 me: working on PoWER: https://github.com/seraphis-migration/monero/pull/230, have been testing hardware for difficulty parameters 18:06:02 I noticed the situation is a little more complex, there is [#9915](https://github.com/monero-project/monero/pull/9915) which fits in between "updated Wallet API" and "simplewallet without wallet2 based on Wallet API", but I did a lot of rebasing and cleaning up and I think I'll be able to push soon 18:06:05 Altogether too many possibilities with those commits and PRs :) 18:07:02 Howdy 18:07:14 And for now I would focus on master branch only, and then add PRs for release once the one on master is merged 18:07:38 me: got `fcmp++-alpha-stressnet` branch to compile on Ubuntu 18 with GCC 7 18:07:41 me: finished up deadlock PR upstream, finished first round review on tx relay v2 (will work with 0xfffc on further changes this week), tested and integrated kayabanerve's new change to the FCMP++ lib that brings memory usage of FCMP++ verification down to 250mb per call to verify (down from ~800mb), started looking into an apparently simple cause of slow sync in the stressnet ri ght now (should have a fix for that out today) 18:08:38 Did a massive overhaul of the Carrot device interface which should help HW devs integrate and prepare for Hybrid/Carrot hierarchy account derivation integration 18:09:22 Writing up the docs soon 18:09:43 SNeedlewoods: Not a good idea to include what is now in #9915 in the "Updated Wallet API" PR as well? Or, a good reason to keep that separate? 18:10:39 jberman: 250 MB sounds downright reasonable :) 18:12:11 agree, 250mb is solid. Unfortunately it's still doing lots of small allocations (as discussed last week), and so OOM's are still plausible for same reason as discussed (a machine with lots of malloc arenas may keep freed memory allocated / not release back to the OS). Someone reported an OOM on an 8gb 4 core machine running with the 250mb change 18:12:52 I thought about that, but because I already made a PR for it and it's a pretty small PR compared to the Wallet API update and it's relatively separated from it, I concluded to use it, but I'm open to suggestions, so if you think I should close #9915 and add it to #9464, I can do that 18:13:38 As far as I understand, a submitter is indeed free to close a commit, if reasons appeared to do so since it was opened. 18:14:10 lmk when it's ready :) 18:17:38 jeffro256 you raised the argument that since many small allocations = the bane of most memory allocators, we should prioritize allocating all memory before in a large slice before mainnet. One relevant point: I also tested using jemalloc and mimalloc on the rust side as the memory allocator instead of the default system allocator (which is glibc on most systems), and found that th ose memory allocators did not have the issue of increasing RSS, and that they were releasing already freed memory 18:18:20 Interesting 18:18:26 But using one of those memory allocators would introduce a pretty significant dependency (jemalloc isn't maintained anymore, and mimalloc is microsoft) 18:18:47 Did you do performance checks too? 18:18:49 that was a main reason why I felt going with a simpler option to tune glibc makes solid sense 18:18:57 It might be a tradeoff 18:19:09 no, but we do have a framework set up! I can check that out too 18:19:45 I think significant performance differences in something lie allocating would be a bit surprising 18:19:49 it is interesting that kayabanerve 's latest change speeds up 128-input verification by around 10% on my machine, which does suggest the allocations may be having a perf impact 18:20:13 It would be interesting to see those results IG, but like you said, it would probably be difficult to recommend adding this new allocator as a dependency 18:21:32 I remember Koe saying that during a final tuning of the Seraphis lib, he sped it up by 5-10% by simply adding 'reserve()' statements where applicable 18:21:58 You mean for things like vectors? 18:22:10 Yup 18:23:52 One reason I bring up jemalloc / mimalloc now is also to note how it lends some support to just tuning glibc / that being an acceptable first step, since other memory allocators do seem to handle releasing memory fine (we also have some mac and windows testers in the stressnet channel) 18:24:46 With "tuning" you mean using those "arenas" in the best possible way? 18:25:08 https://github.com/seraphis-migration/monero/pull/228 18:25:35 that PR sets max n areans equal to the number of threads on the machine, so there is 1 arena per thread 18:26:18 which from the docs, sounds like it would even benefit performance and avoid contention since some machines may default to having fewer than 1 arena per thread 18:26:57 (and of course, our issue is that some machines seem to allow more than 1 arena per thread) 18:27:19 Sounds like many tests will be needed to be sure about the behavior in all reasonable circumstances ... 18:27:49 That's fair, I just want to point out that the tuning may or may not come with CPU time tradeoffs. 18:28:56 I wonder how many arenas a typical Linux + GNU system will use for a 128 thread CPU 18:29:27 I think solid perf observed in stressnet (+ no OOM's reported by anyone with that PR in) should be considered major support 18:29:51 I'm excited for v1.5 that includes this + other sync improvements + potentially tx relay v2 18:30:18 that includes the improved memory profile / threading approach 18:30:34 I think this PR is good: https://github.com/seraphis-migration/monero/pull/224 18:31:14 "omeone reported an OOM on an 8gb 4 core machine running with the 250mb change" Was that with or without that #228 arena change? 18:31:23 without 18:31:29 and they're on a linux 18:31:56 Ah, ok, that's somewhat reassuring 18:32:46 Because 8 GB is pretty decent, I don't think we make many friends with a Monero daemon that does not run in such a config ... 18:33:25 For Linux at least 18:33:27 2 or 8 if I understand this correctly 18:33:29 > The default value for the M_ARENA_TEST parameter is 2 on 18:33:31 systems where sizeof(long) is 4; otherwise the default 18:33:33 value is 8. 18:35:18 So next step is that v1.5 with everything significant in, and the hope that it will work pretty well already? 18:35:40 yep 18:36:07 That will get interesting :) 18:37:38 So maybe we limit the arenas IFF default is greater than thread count , and available RAM is relatively low? 18:39:23 this does sound reasonable to me 18:41:21 Alright, that seems to converge towards a plan. SNeedlewoods , do you want to go back to your PRs again for some more clarifications, or do you feel you there is enough clarity? 18:43:16 Maybe someone can give their opinion if I should merge the "remove cached password" into "updated Wallet API" so we are at 2 PRs 18:44:05 so close this https://github.com/monero-project/monero/pull/9915 18:44:07 and add the code to https://github.com/monero-project/monero/pull/9464 18:44:56 Would certainly make moving it over to release simpler - if that will even become necessary, depending on timeline 18:45:07 Less PRs, less problems 18:45:46 funny.. I was about to say the opposite. Before looking much deeper into it, it may make it easier to review and get this code across the line if "remove cached password" was done first, and then adding functions to the wallet API was done building on that. In fact it might be easier if you split multiple PR's like that 18:46:20 and that also brings me to discussion of upstreaming PR's for FCMP++ / Carrot. I think it would help to re-discuss strategy for that too 18:46:39 remove cached password came after and is also based on #9464 18:47:05 It needs things in that PR, i.e. is not stand-alone 18:47:25 right, so maybe would work to isolate that feature as its own PR first 18:50:03 So I would do it this way 18:50:05 1. PR = 2 commits: "Wallet API round one" + "Wallet API round two" 18:50:07 2. PR = 1 commit based on top of 1. PR: "remove cached password" 18:50:09 3. PR = 1 commit based on top of 2. PR: "replace wallet2 with Wallet API in simplewallet" 18:51:40 the reason I bring up splitting / isolating a feature like "remove cached password" is that it's a nice change in its own right, that would be a lot easier to review as its own standalone piece. So you could break off that feature and make only the wallet API changes necessary for that change 18:51:58 that reduces the diff size for modifying the wallet API and brings a nice easier-to-review feature into the code sooner 18:52:55 You propose to switch the order of 1. and 2., and make that possible by just dragging along the necessary base changes from 1. to make that possible? 18:53:02 yes 18:53:23 and then using that strategy for other changes to the wallet API 18:53:38 iirc that may be just one method `verifyPassword()` 18:53:49 For possibly even more, smaller PRs, with better "reviewability" 18:53:55 right 18:54:22 that's basically how I've started with FCMP++ PR's, although that's kind of blocked at this point 18:54:38 Well, I am not sure. It's not that terrible, because it's mostly quite straightforwards stuff. I had no problems to review 1. , round 1, in any case 18:55:22 fair counter argument 18:55:49 I also think about people later, that just want to see when and how Wallet API was extended, and it may be comfortable for *them* to have it "all in one" 18:56:20 that sounds like a good idea to me in regards to Wallet API, but not sure if that approach works as well for 3. PR simplewallet 18:56:21 or we would have `m_wallet` (old wallet2 object) and `m_wallet_impl` (new Wallet API object) at the same time 18:57:16 Oh, simplewallet using both is a bit strange, no? 18:57:24 I think this strategy is ok considering rbrunner7 's arguments 18:57:30 also good point 18:58:01 it would only if we split it up, now it does not 18:58:22 Alright, I think we can "sleep over it" for a night :) 18:58:46 thanks for all the feedback, appreciate it a lot 18:59:14 we can revisit this discussion next week as well. Will work primarily on getting to the big v1.5 this week 18:59:24 Do we have anything other left to discuss? Nearing the full hour ... 19:00:43 (worth updating I've also started dabbling in Serai a bit too, I am hoping to pick up more time on Serai soon enough) 19:01:41 Ok, seems we can close the meeting for today. Thanks everybody for attending, read you again next week! 19:02:13 thanks everyone 19:02:19 As I said in a GH comment, my PR minimizing RAM may aggravate glibc less but does not fix glibc. 19:03:13 jberman @jberman:monero.social: We prior iterated over 1000 empty vectors and checked they were empty 99% of the time. We know immediately iterate over the non-empty vectors. That likely explains the speedup. 19:03:42 fair