Time Nick Message 02:05 paramat thanks for exposing chunksize on mapgen init, this will make lua mapgens a little simpler and faster 05:42 VanessaE celeron55: *poke* 05:43 VanessaE celeron55: those domain name issues (or whatever it was) that you had just recently... would they be responsible for failed forum redirects? 05:43 VanessaE e.g. http://minetest.net/forum/viewtopic.php?t=2334 should point..somewhere (it's one of cornernote's posts) but goes off into 404-land 06:52 Zeno` hmmmm, are you here? 06:52 hmmmm don't ask to ask, just ask 06:52 hmmmm if i wasn't here i'd surely see it in the logs 06:52 Zeno` I don't want to ask anything 06:52 Zeno` I just wanted to know if you were here 06:52 hmmmm :/ 06:53 hmmmm i'm always here 06:53 Zeno` https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_env.cpp#L540 <-- why is that leaking? 06:53 hmmmm so you did need to ask 06:53 Zeno` no, I thought of this just then 06:53 Zeno` :P 06:54 hmmmm that exact line is 'causing a memory leak'? 06:54 hmmmm i simply don't see it 06:54 Zeno` 539-542 are, yes 06:55 Zeno` I can't either, but if I comment those line out all leaks go away 06:55 hmmmm all three lines? 06:55 Zeno` Because I could not see it I tested this 4 times to make sure 06:56 Zeno` well, just 540 and 541 will do 06:56 hmmmm how about commenting out 540 06:56 hmmmm does it still show a leak then? i guess that'd make sense 06:58 Zeno` yeah quite slow. About 1MB per 10 minutes (very roughly) 06:58 hmmmm doesn't it tell you where the memory being leaked gets allocated? 06:58 Zeno` If I comment out just line 540 I get Lua errors 06:58 Zeno` yes 06:58 Zeno` line 540 06:59 hmmmm it's being *allocated* there? 06:59 Zeno` it's being pushed onto to the lua stack and apparently never garbage collected 06:59 Zeno` one sec, I'll tell you where it's allocated 06:59 hmmmm did you compile it with -O1? 07:00 hmmmm try recompiling with -O0 and try your analysis again 07:00 Zeno` -O3 07:00 hmmmm yeah, well 07:00 hmmmm what do you expect 07:00 Zeno` I don't expect a memory leak 07:00 hmmmm it's giving you a bullshit line number because you optimized it so much 07:00 Zeno` massif is meant to be used with release builds 07:00 Zeno` no it's not, I'm using -g 07:01 hmmmm well 07:01 hmmmm i have no clue 07:01 Zeno` hmm :( 07:01 hmmmm just try with -O0 ... i guarantee you'll get somewhere 07:01 Zeno` I guarantee it will be the same line, but I will just to keep everyone happy :) 07:04 hmmmm for what it's worth, if it's "apparently never garbage collected" (not necessarily lost all references) it could just be a faulty mod you're using 07:04 hmmmm or if you're using the default minetest_game it could also be the tree leaf cache 07:05 Zeno` not using mods 07:06 Zeno` but I am using minetest_game 07:07 Zeno` running test for 20 minutes 07:08 Zeno` it's quite possible minetest_game caches it, that's true 07:09 Zeno` if it's a bug in minetest_game I'm happier. But, 1MB every 10 minutes is still a bit worrisome for a server that runs 24 hours 07:09 hmmmm should try setting false in minetest_game/mods/default/functions.lua:248 07:10 Zeno` even worse for one that runs longer (e.g. one that does not do daily restarts for backups ) 07:10 Zeno` ok, I'll let this test finish first though 07:15 Zeno` should leaf decay matter? I'm deleting the db every test and just letting the client move forward constantly 07:16 hmmmm well the leaf decay cache is one thing that holds onto the return value of find_node_near 07:19 Zeno` line 149 quite possibly has the same issue as well 07:20 Zeno` but I've only been looking at one think at a time 07:22 Zeno` I'll stop the test now 07:25 MinerDad https://github.com/minetest/minetest/issues/1640 appears to be working as designed. He is trying to modify the wielded itemstack with add_item but returns an unmodified itemstack which replaces the modified one. 07:26 Zeno` I guess I have to run for 60 minutes 07:26 Zeno` it's running about 4 times slower 07:26 Zeno` line 540 is still there but 149 is not 07:27 Zeno` seriously, the line numbers do not change (you also run callgrind and cachegrind with -O2 or -O3 builds... you just need -g) 07:53 hmmmm -O1 causes a lot of problems when i'm trying to debug 07:54 hmmmm one of the biggest issues is that the line numbers do not match up. i commonly find myself trying to figure out what's wrong with a certain line of code and then i realize later on that the line of code where the problem actually exists is like 30 lines earlier 07:57 Zeno` hmmmm, that's in gdb 07:58 Zeno` and why I asked about a week ago why debug builds use -O1 07:58 Zeno` they're basically useless for debugging, you need -O0 08:01 Zeno` I'm going to make a world using just minimal 08:02 Zeno` I don't want to be hunting for a bug that may or may not be in minetest_game. My data is faulty (thanks for making me realise it) 08:06 Zeno` hmmmm, people think that compiling debug builds with -O1 is necessary otherwise it's too slow for anybody to run. My argument is that if -O0 is too slow to run then there is something very, very wrong 08:06 hmmmm for me, it's not too slow, it's just uncomfortably slow 08:07 Zeno` maybe 08:07 hmmmm and luajit version detection is broken on freebsd 08:07 hmmmm yeah but zeno i have a xeon e3-1230v2 08:07 Zeno` I don't use luajit for my test builds now 08:07 Zeno` it just breaks 08:07 hmmmm i'm pretty sure the average person has a slower processor 08:08 hmmmm it just didn't run at all on my last computer (athlon 64 3500+) 08:08 hmmmm but also since then, minetest substantially improved in terms of performance 08:08 hmmmm and there's still a lot more optimization that has yet to be done 08:08 Zeno` yeah, but -O1 does hardly any optimisations. It corrects yucky coding for the most part. BUT I am basing this experience on my experience with C and not C++; therein may be the difference 08:09 Zeno` oh, there's heaps to optimise. Most of it not terrible enough to bother with though :) 08:10 Zeno` especially when any modern compiler will "fix" it anyway :) 08:10 Zeno` bugger hand optimising most loop invariants, unrolling, aligning bytes and all that crap 08:11 Zeno` may as well program inner loops in ASM if you go that far heh 08:12 Zeno` have you tried -O0 recently, by the way? 08:12 hmmmm i had to use it for debugging some settings problems 08:13 Zeno` I can almost play the game when it's running under valgrind now (with -O0). Well, "play" if 2-3 fps is ok 08:14 Zeno` ok, maybe "almost" is an exaggeration 08:14 Zeno` who wants to actually play with debug builds anyway? Can cmake handle more than two build types? 08:14 Zeno` i.e. more than RELEASE and DEBUG? 08:16 hmmmm i'm sure it can 08:16 hmmmm but i'm trying to figure out if there's value in -O1 08:20 jin_xi so if anyone needs the ultimate minimal game for testing, here it is: http://ge.tt/6gcpoz62/v/0 08:20 jin_xi absolutely NO features 08:22 Zeno` hmmmm, does adding -g help with -O1 and gbd? I've never tried that 08:22 Zeno` gdb* 08:22 Zeno` jin_xi, is that serious? 08:23 Zeno` For debug builds I have never found value in -O1 08:23 Zeno` but if -g helps with the line numbers being correct then maybe I'm wrong 08:24 Zeno` oh dear. -g crashed his computer 08:25 jin_xi Zeno`: semi serious, minetest minimal game is far from minimal so i wanted to see the engine mostly on its own... 08:26 Zeno` ok, well the leak I was seeing before is apparently not here with minimal 08:26 Zeno` so it's a minetest_game issue 08:26 Zeno` there is still one leak to fix though :( 08:43 Zeno` ok, I need help with it 08:45 Zeno` https://github.com/minetest/minetest/blob/master/src/map.cpp#L2305 08:47 Zeno` maybe instead of copying them, I can apply them 08:48 Zeno` The main problem seems to be that by copying them the blocks are not added to the changed_blocks queue 08:49 Zeno` modified_blocks* 09:07 Zeno` ugh :( 09:17 celeron55 VanessaE: oh, that problem is caused by me moving minetest.net to another host 09:32 celeron55 i think it works now 09:43 Zeno` is raiseModified(MOD_STATE_WRITE_NEEDED... kind of equivalent to marking a block as modified? 09:51 Zeno` Ahh neo, so nice of you to join us 09:53 Zeno` neoascetic, #1975 09:53 ShadowBot https://github.com/minetest/minetest/issues/1975 -- Fix OSX packaging finally by neoascetic 09:53 Zeno` you added line comments to your own PR? 09:54 Zeno` Also, in answer to "Does anyone use this makes target at all", Yes. You do :) 09:54 Zeno` make's 09:57 celeron55 any further comments from anyone else to this? https://github.com/minetest/minetest/pull/1963 09:58 Zeno` celeron55, my only comment is that more documentation is good 09:58 Zeno` so long as it's useful, but you've already addressed that I think 09:59 Zeno` neoascetic, so your PR is needed for OSX? 10:00 Zeno` neoascetic, and is it correct (I cannot test) 10:00 celeron55 (more documentation isn't good if it's factually wrong or does not actually usefully describe the code) 10:00 Zeno` celeron55, I agree 10:01 Zeno` I haven't looked through the latest changes in detail but I looked through them yesterday or the day before 10:01 Zeno` If they are not correct, useful and factual then I say add them 10:02 Zeno` celeron55, why can't neoascetic speak here? 10:03 Zeno` nvm, worked it out 10:04 celeron55 i'm still having second thoughts about this way of using doxygen 10:04 Zeno` I don't really like the doxygen style that PR uses, but maybe it's just persona; 10:05 celeron55 the issue is that moving the method descriptions to the class comment moves them so in the code, and then they aren't there in the code where you would most naturally read them 10:05 Zeno` yeah, I don't think I like that 10:05 Zeno` I'm just looking at it now 10:05 celeron55 but if you read all my comments in the PR, you see why it's that way in that case 10:06 Zeno` I do see the issue 10:06 celeron55 (there are two virtual interfaces into one class and the documentation gets weirdly split if done otherwise) 10:07 Zeno` conundrum 10:07 celeron55 splitting the documentation into each of the virtual interfaces would make the C++ look reasonable but doxygen output considerably harder to navigate 10:07 Zeno` surely there is a doxygen option so that's not needed 10:08 celeron55 i wouldn't be so sure; can you find anything useful? 10:09 Zeno` looking now 10:10 neoascetic finally, I am here 10:13 neoascetic What about https://github.com/minetest/minetest/pull/1975. Currently "make package" does not work well on OSX at all. If this patch will be applied, OSX users will be able to install minetest with one simple command via Homebrew 10:13 Zeno` I have no problem with that patch if it's correct and the standard way of doing things 10:14 Zeno` But, as I said, I cannot test and I am not sure who can 10:15 Zeno` celeron55? 10:16 neoascetic https://github.com/Homebrew/homebrew-games/pull/187 it was tested by homebrew autotest system. Probably @smarek @mdoege may help 10:16 neoascetic If this will be accepted, there will be no need in separate project by mdoege (https://github.com/mdoege/mtmake-osx) 10:16 Zeno` neoascetic, I'm just not sure how to proceed here. If you say it's correct I will believe that but I don't know how I can merge it, since no other coredev seems to be able to test (and neither can I) 10:17 neoascetic So we probably need to hear feedback from mdoege and smarek? 10:17 Zeno` I'm really not sure which is why I'd like the feedback from another core dev or celeron55 10:18 Zeno` celeron55, can you buy me an OSX laptop? 10:18 Zeno` :) 10:18 Zeno` lol 10:19 celeron55 i wonder who is using the OSX !RUN_IN_PLACE build to begin with 10:20 celeron55 if it's completely useless currently and this makes it useful and apparently this does not touch other builds, i guess it's fine? 10:20 celeron55 it seems this might unintentionally break a cpacked run_in_place build; is that a thing? 10:22 Zeno` in porting? 10:23 celeron55 these changes apply to run-in-place and non-run-in-place: https://github.com/minetest/minetest/pull/1975/files#diff-af3b638bc2a3e6c650974192a53c7291L216 10:23 celeron55 none of the others do 10:24 celeron55 it's not a problem if nobody is using it but i have no way of knowing 10:24 neoascetic SHAREDIR is always local dir on APPLE 10:25 neoascetic What is "official" way to make a new release archive? "make package" or something else? 10:26 celeron55 on windows it's "make package" 10:26 celeron55 on linux binary package creation is not centralized; each distribution does it its own way 10:27 neoascetic As far as I know CMake may build packages for debian and rpm 10:29 celeron55 it doesn't make sense to use it for those as there are many other packaging systems too and each distribution uses a slight variant of those 10:30 celeron55 and... frankly, if we don't need to do it ourselves, we don't 10:30 Zeno` I'll merge it if celeron55 doesn't yell and you can confirm it's correct 10:31 Zeno` I can't test and it doesn't seem anyone who's been here in the last few weeks can, so *shrug* 10:31 celeron55 i guess it's fine then 10:31 Zeno` it doesn't break anything for me on Fedora 10:31 neoascetic I confirm that it far from ideal but at least doesn't break anything :) and works on my end 10:32 Zeno` we still have a week before release, so I'll take the risk ;) 10:32 celeron55 neoascetic: make sure to look for complaints from other osx users and try to fix them if possible 10:32 celeron55 (we have no means and no interest in doing it ourselves) 10:32 neoascetic yeah, I am interested in this too 10:33 Zeno` yes, after it's merged please try and get feedback and correct anything that comes up 10:33 neoascetic next what should be fixed is to copy dynamic libraries to app bundle via cmake's fix_bundle or something 10:33 neoascetic but at least this PR adds ability to install minetest via homebrew 10:34 neoascetic (its package manager for osx) 10:34 Zeno` merged 10:34 neoascetic Ok, thx, I'll update related homebrew-games PR 10:36 Zeno` and I've fixed the memleak, but I'd like comments: https://gist.github.com/Zeno-/e9a95468ef61159660fb 10:36 Zeno` the other memleak* 10:36 Zeno` this is the smaller one, the big one I've already fixed 10:39 Zeno` Before patch shown in my gist: http://i.imgur.com/GrhbLtG.jpg and, after: http://i.imgur.com/Yv5WTC1.jpg 10:41 celeron55 what does that even do 10:42 Zeno` it's copying blocks to a new "instance" but the blocks where flowing stuff is present were not being marked as needing updates 10:43 Zeno` so they were just sitting there in the queue never being processed; next time it happens the queue grew large 10:43 Zeno` larger* 10:44 Zeno` I suppose if a player happened to trigger the flowing it would have gone away but I didn't test that 10:44 celeron55 what is the prev_block thing 10:45 celeron55 it seems like a hack or completely unneeded 10:45 Zeno` I'll add an assert but I am pretty sure that two v316's do not need to be in the same block and often are in the same block 10:46 celeron55 they are block positions 10:46 celeron55 not node 10:46 celeron55 and it wouldn't work anyway in the way you wanted 10:46 Zeno` ok, well I was being paranoid. I'll add an assert and remove it if they are always unique (for each iteration) 10:46 Zeno` a temporary assert* 10:46 celeron55 that's stupid, never do that 10:47 Zeno` never do what? 10:47 celeron55 code defensively like that when you don't know what is happening; just find out what is happening instead 10:48 Zeno` yes, it's not finished which is why it's a gist for feedback rather than a PR ;) 10:48 Zeno` I've spent a couple of hours going through all this code and need to spend maybe an hour more (i've never looked at this section before) 10:49 Zeno` just want to get things straight in my head 10:49 celeron55 but 10:49 celeron55 this wasn't yet clear: what queue grew larger? 10:50 Zeno` m_transforming_liquid 10:50 Zeno` this is copying a block 10:50 celeron55 do you mean the only place where that is cleared is where the block is written to disk? 10:50 celeron55 that doesn't sound right at all 10:51 Zeno` but the block is never marked as "dirty" 10:51 Zeno` no it's cleared in asyncstep 10:51 Zeno` but the copied blocks are not marked as needing updates 10:51 Zeno` so they're ignored 10:51 celeron55 also, that isn't copying a block but instead it's copying the map generation result to blocks (and at that point it's ensuring that they exist so that it can then copy the stuff to them) 10:52 celeron55 Zeno`: needing what updates? 10:52 celeron55 has this code changed in some way i am not aware of or are you misinterpreting it very wrong 10:54 Zeno` you may do the massif checks yourself if you like ;) I may be using the wrong terminology but I call a loop that moves an entire UniqueQueue from one object to another object a copy. Perhaps move would have been more accurate 10:55 Zeno` I don't think it's changed recently. Was the leak introduced recently? 10:55 celeron55 there's no reason whatsoever you should be doing it like this, this is nonsense 10:57 Zeno` doing what? 10:57 celeron55 so is the problem that Map::m_transforming_liquid is growing infinitely or is the problem that for some reason MapBlocks are never unloaded? 10:57 celeron55 what is the thing that is taking up the memory 10:57 Zeno` the problem is that Map::m_transforming_liquid is growing infinitely 10:57 celeron55 how fast 10:58 celeron55 is the problem that Map::transformLiquids is rate-limited and thus is not keeping up with the amount of stuff added to the queue? 10:58 Zeno` The only place it shrinks is in Map::transformLiquids() 10:58 Zeno` and that is not called unless the mapblock is somehow marked as dirty 10:59 celeron55 no that's wrong 10:59 Zeno` no, it's never being called because the newly created blocks are not "dirty" 10:59 celeron55 it's done for all blocks that have queued liquid updates 10:59 Zeno` where else is stuff popped from the queue? 11:00 celeron55 nowhere else, and nowhere else they should be popped 11:00 Zeno` void Server::AsyncRunStep(bool initial_step) is the only place it's called from 11:00 Zeno` and it is not called for blocks that are not marked as dirty 11:01 celeron55 where is that check 11:01 celeron55 i can't find it 11:01 Zeno` what check? 11:01 celeron55 where is it checked whether the blocks that are being handled in Map::transformLiquids are dirty? 11:01 celeron55 there is no such check; everything is handled 11:01 Zeno` they have to be in the modified_blocks queue 11:01 celeron55 no, that's the result queue 11:01 Zeno` server.cpp line 492 11:01 celeron55 it's output from the function 11:02 celeron55 now, how about you just increase your liquid_loop_max setting and see your memory problem disappear? 8) 11:03 celeron55 i don't understand how your "fix" "fixes" that, it's some kind of coincidence and unintended behavior 11:03 Zeno` on line 646 of server.cpp, transformLiquids(modified_blocks); is called 11:04 Zeno` how do these transforming blocks that have liquids to transform get into the modified_blocks queue? 11:04 Zeno` they don't unless the block is somehow marked as dirty :/ 11:04 celeron55 it's output from Map::transformLiquids, NOT input to Map::transformLiquids 11:04 celeron55 OUTPUT, MAN 11:04 Zeno` it is input 11:04 Zeno` look at the line 11:04 Zeno` oh 11:04 celeron55 holy fuck i'm out 11:04 Zeno` but anyway 11:05 Zeno` ffs :/ 11:05 celeron55 i coded the whole damn thing 11:05 Zeno` I know 11:05 celeron55 just read what i say 11:05 Zeno` but there is something wrong 11:05 Zeno` if it was correct it would not leak 11:06 Zeno` perhaps we both need to step away for a second. I cannot think atm 11:06 celeron55 i can think very clearly and can repeat what i said 11:06 celeron55 Map::transformLiquids eats stuff from Map::m_transforming_liquid and is RATE-LIMITED. that is your problem 11:06 celeron55 increase the rate and problem disappears 11:07 celeron55 it's way simpler than you thought 11:08 Zeno` but it never gets freed 11:08 Zeno` how fast does the rate have to be? 11:08 Zeno` after 24 hours it's not freed 11:08 celeron55 for some reason your server is constantly generating more queued updates than the handling rate limitation is 11:09 celeron55 that reason isn't important 11:09 Zeno` my patch still results in it being rate limited anyway 11:09 Zeno` so I don't see the problem 11:09 celeron55 probably by missing some updates by whatever illogical logic 11:09 celeron55 i.e. breaking it 11:10 celeron55 try this: 11:10 celeron55 change this: if(loopcount >= initial_size || loopcount >= loop_max) 11:10 celeron55 to this: if(loopcount >= initial_size) 11:10 celeron55 in Map::transformLiquids 11:10 celeron55 then it's not rate-limited 11:10 Zeno` ok 11:10 Zeno` let me stash changes first 11:11 Zeno` what line #? 11:12 Zeno` nvm, found it 11:13 Zeno` ok, need to do the 30 minute test now. I'll bb after that 11:14 celeron55 later we should probably try to find out why it's queuing liquid updates at such a fast rate; it could be completely valid behavior though 11:48 Zeno` lol, well at least it's obvious now 11:48 Zeno` if liquids are queuing at 10/second and the most the rate limiting allows is 5/second then of course it will infinitely grow 11:50 celeron55 the rate limit asssumes that if the queue is large, it is that way because something dumped a huge amount of stuff in it but nobody is continuously adding a large amount of things to it 11:50 celeron55 and that's the situation in which it was made to help with server delays; in this case it's a problem 11:51 Zeno` so it's dependant upon the CPU execution speed 11:51 celeron55 ? 11:51 Zeno` because mapgen is constantly adding crap to that queue 11:52 celeron55 if the stuff is added by the mapgen, then i think there could be a mapgen quota which is added to the regular rate limit so that it is always surely processing at least everything that the mapgen queues 11:52 celeron55 because that's kind of important 11:52 Zeno` possibly u16 loop_max = MYMAX(g_settings->getU16("liquid_loop_max"), m_transforming_liquid.size() * 3 / 4); is better (0.75 chosen at random) 11:52 celeron55 but this isn't a fool-proof solution either 11:52 celeron55 dynamic things like this are hard to control 11:53 Zeno` but it at least limits the maximum queue size 11:53 celeron55 i don't think that's a good idea; that will break the intention of what the rate limit was intended to do 11:54 celeron55 more like 1/100, but it's bad too 11:55 Zeno` I don't know of another way to dynamically account for when stuff is being added to the queue > it can be processed (0.75 may not be the best I chose chose that at random) 11:55 celeron55 something like if(loopcount >= initial_size || (loopcount >= loop_max && m_transforming_liquid.size() < MAX_LIQUID_QUEUE_SIZE)) might work 11:56 Zeno` but then loopcount >= loop_max && m_transforming_liquid.size() < MAX_LIQUID_QUEUE_SIZE will always be true for those CPUs that can keep up 11:56 Zeno` need to shave a bit off 11:56 celeron55 then it will drag somewhat badly if the rate limit is too low but it will behave 100% correctly in the more important burst-smoothing function 11:56 Zeno` MAX_LIQUID_QUEUE_SIZE needs to, at least, be based upon g_settings->getU16("liquid_loop_max") I think 11:57 Zeno` or maybe a function of both MAX_LIQUID_QUEUE_SIZE and g_settings->getU16("liquid_loop_max") 11:57 celeron55 it could be multiplied by 10 or 100 or something 11:57 Zeno` maybe 11:57 celeron55 like, that would be the maximum allowed smoothed burst size 11:58 Zeno` yeah, that makes sense 11:58 Zeno` multiplied by 8 maybe so we can get an extra .00000000000000000000000000001ms 11:58 Zeno` but 10, *shrug*... whatever 11:59 celeron55 maybe this could have some dynamic adjustment where it will always clear the queue to size()=0 if it is detected to never get cleared up when being rate-limited 11:59 celeron55 but problems in those are hard to diagnose 11:59 Zeno` well that's why setting loop_max is probably the better way 11:59 Zeno` because it pops more than it needs to 11:59 celeron55 speaking about that: it should be read as S32 or U32 12:00 celeron55 U16 isn't large enough for some values needed for this 12:00 Zeno` true 12:01 Zeno` if it's 10x it shouldn't degrade server performance too much 12:01 Zeno` and probably degrade it less than huge memory fragementation due to a slow leak 12:02 celeron55 i don't know if 10x is large enough 12:02 celeron55 it only spreads stuff to 10 server steps 12:03 Zeno` I'll try u32 loop_max = MYMAX(g_settings->getU16("liquid_loop_max"), g_settings->getU16("liquid_loop_max") * 16); and see what the memory profile looks like 12:03 celeron55 what, that doesn't change anything 12:04 Zeno` sorry, pasted wrong line 12:04 celeron55 you sound like you should sleep or something 8) 12:04 Zeno` you know what I meant 12:04 celeron55 make it log the values so that you don't have to guess if it's doing what you intended 12:13 kahrl I guess another option would be to try to optimize transformLiquids so it can handle higher liquid_loop_max 12:13 kahrl for example it makes many node id lookups that seem unneeded 12:13 Zeno` yeah I'll add the log after this next test (i'll have to re-form my condition to do so) 12:14 Zeno` kahrl, that would be beneficial as well but would still not account for the dynamic nature of cpu execution speed 12:14 kahrl in map.cpp:1696 the nodemgr is queried for some liquid_type, then inside the switch there is an if (the node == CONTENT_AIR) 12:15 kahrl which could be optimized by simply handling the CONTENT_AIR case outside of the switch 12:15 kahrl and all the queries for liquid_alternative_* could be optimized by storing the result of that lookup in ContentFeatures 12:16 Zeno` another option would be to check how much the queue size is "outrunning" things but that would not be re-entrant (in the simple implemention) 12:16 kahrl I don't think reentrancy is a concern here 12:17 Zeno` it's not 12:17 kahrl transformLiquids is called sequentially 12:17 Zeno` unless another metric was added :) 12:17 Zeno` hmm, it is *now* 12:17 Zeno` oh wait 12:17 Zeno` it's from asyncstep 12:17 Zeno` ok, that might be another option then 12:19 Zeno` I have a gut feeling that processing the entire queue every x cycles (be it 10 minutes or 30) is not going to be a concern compared to increasing memory fragmentation and therefore cache misses, but I'm not sure yet 12:19 Zeno` maybe it's only an issue now because the whole game is faster :P~~~~~ 12:21 kahrl Zeno`: before the limit was added, it was possible to completely lock up a server by strategically placing liquid sources 12:22 Zeno` yes, but this should still be limited 12:22 celeron55 what if it measured the average number of queued updates per, say, 10 seconds and processed at max the amount per step corresponding to 3 times the incoming rate per 10 seconds 12:23 celeron55 then if the rate doesn't vary a lot, it would always clear it, and never would it increase infinitely, and if there are large bursts, they would be smoothed out almost as well as ideally possible 12:23 celeron55 s/the average number of queued updates/the average number of added queued updates/ 12:24 celeron55 this would seem to me like a simple but effective limiting scheme 12:24 celeron55 or, well, smoothing scheme 12:25 celeron55 it might still be possible to lock the server though; in those cases it doesn't make sense to store infinite-length queues but instead it should drop the queue if it becomes way too much to handle 12:25 celeron55 it's always a problem though 12:25 celeron55 but, like, if it's taking a minute to process, it should just delete it 12:26 celeron55 no options there 12:35 Zeno` ok, testing this 12:35 Zeno` if it works as expected I'll make a gist and then clean it up based on comments 13:06 Zeno` http://i.imgur.com/1NXIfR7.jpg much better 13:12 Zeno` #1979 13:12 ShadowBot https://github.com/minetest/minetest/issues/1979 -- Do not allow the m_transforming_liquid queue to increase until all RAM is consumed by Zeno- 13:14 celeron55 over_queue_dump_sz? 13:14 celeron55 (~(u32)0) / 4? 13:14 celeron55 why do you write so hard-to-read code 13:15 celeron55 i am starting to feel like you are some kind of a microsoft agent who gets paid for sabotaging the MT codebase 13:19 Zeno` Sorry for fixing it 13:19 Zeno` Sorry for making in 20x faster 13:19 Zeno` how hard is ~0 to understand? 13:20 Zeno` it's 0 but with all bits set to 1 13:20 celeron55 i guess i'll force myself to be fine with that if you move over_queue_dump_sz to be a member of Map like it should be and change the name to be understandable in that context 13:21 Zeno` I'm not going to comment that :/ 13:21 Zeno` ok, I'll move it 13:22 Zeno` what's a better name? queue_excess? 13:22 celeron55 maybe m_transforming_liquid_loop_count_multiplier 13:22 Zeno` well, I was trying to keep it shorter but at least that's self documenting :D 13:23 celeron55 sometimes you need a long name to make it understandable 13:23 Zeno` yes, that's fair enough 13:23 kahrl why is it still getU16? 13:23 celeron55 also, change g_settings->getU16 to getS32 and don't call it twice because it's one of the slowest things to do 13:25 celeron55 increasing the multiplier at x2 intervals is somewhat too fast but i guess it's tolerable for now 13:25 celeron55 something like x1.1 would work better 13:26 celeron55 doubling it every step will very quickly diminish the smoothing effect 13:27 Zeno` doubling is fine I think 13:27 celeron55 also, by immediately decreasing the multiplier if it isn't needed, without any threshold, you make it usually not fully clear the queue 13:28 Zeno` why getS32() ? 13:28 celeron55 you should probably only lower the multiplier if lowering it will still allow it to fully clear the queue 13:28 celeron55 (you yourself said that you'd like it to do that to prevent memory fragmentation) 13:29 Zeno` I mean, the setting is not s32 13:29 celeron55 there's no getU32 8) 13:29 celeron55 and settings have no type 13:29 Zeno` I'll make it s32 then and let it cast 13:31 Zeno` decreasing by multiples of 1.1 may take a long time to get things under control again 13:31 Zeno` I prefer the binary approach 13:31 Zeno` (2) 13:34 Zeno` if you add the prints it goes up very quickly 13:35 Zeno` with just one block emerging 13:35 kahrl maybe make it go up by 1.1 but down by 2? 13:36 Zeno` update pushed 13:36 Zeno` kahrl, maybe... but add the std::cout and watch 13:37 celeron55 can you paste a log of it running? 13:38 Zeno` celeron, let me add std::cout << loop_max << std::endl; after line 1650 13:40 Zeno` http://pastebin.com/6dyM0UTb 13:40 Zeno` one client connecting 13:41 Zeno` I think you can see where I connected 13:41 Zeno` client did not move 13:41 celeron55 how large area is it generating? 13:41 Zeno` just stood there 13:42 Zeno` whatever the default is 13:42 Zeno` actually none 13:42 Zeno` the area is already generated 13:42 Zeno` on disk 13:42 celeron55 that value is way too large to make any sense 13:42 Zeno` that's what it is 13:43 Zeno` I've just spent 6 or more hours looking at massif and valgrind crap 13:43 celeron55 your processor barely executes that many instructions in that time :P 13:43 celeron55 where the hell is that queue coming from 13:43 Zeno` if you don't believe the numbers, add some print statements yourself :P~~~ 13:44 Zeno` my processor DOES keep up with that 13:44 celeron55 can you do the same run but also add m_transforming_liquid.size() on the same line? 13:48 Zeno` grrr. ok 13:50 celeron55 it's seeming like something isn't working right and i don't have time to diagnose it now 13:54 Zeno` http://pastebin.com/xpUJpSFW 13:54 Zeno` I'll add a MYMAX 13:57 Zeno` not that it'll make much difference 13:57 celeron55 the problem with this is that this will practically completely break the smoothing effect and this will also break the ability of this code to avoid server lockdown 13:57 Zeno` except to what's printed 13:57 celeron55 this is not good 13:58 celeron55 those simply must both work, otherwise the code is useless 13:58 Zeno` http://pastebin.com/kVb8bxQ4 13:58 celeron55 a better option than what you now have in the PR would be just to remove the rate limit 13:58 celeron55 because there wouldn't be code that isn't doing anything useful 14:00 kahrl > error: expected a type, got ‘std::set::iterator’ 14:00 kahrl why thank you g++, you're so helpful today 14:00 Zeno` pushed update... I even left the cout::'s there (commented out) :) 14:01 celeron55 what if it worked so that it would just limit the processing time to eg. exactly the server step time 14:01 celeron55 that might be ideal 14:01 celeron55 then it cannot hang, but also will process as much as the CPU can practically do 14:02 Zeno` that's a big change though 14:02 celeron55 big? 14:02 Zeno` yes 14:02 celeron55 you just read a timer in two places and compare to a setting 14:02 celeron55 and remove loop_max 14:03 celeron55 it's less code than your current changeset 8) 14:03 Zeno` and then what happens if the cpu cannot keep up? 14:03 Zeno` you add my code back so it can? 14:04 celeron55 what do users want? 14:04 Zeno` it cannot keep increasing because the CPU cannot keep up 14:04 Zeno` the people running servers want this memory leak to go 14:04 Zeno` my VPS leaks, megaF's leaks, VanessaE's leaks, everyone who has a VPS has this leak 14:04 celeron55 it will likely make it go in 99% of cases 14:05 celeron55 the rest is what is the issue, where the server actually cannot keep up with liquids without increasing the step duration 14:05 celeron55 currently it really doesn't even try 14:06 Zeno` no it doesn't. The current code (including mine) can be improved 14:06 Zeno` So we agree :) 14:06 celeron55 it currently does none of the things it should do 14:06 celeron55 so it's pretty bad 14:07 celeron55 if you make make the adjustment upwards to be 1.1x, then it will do one thing (smoothing) 14:07 celeron55 then we still have the DoS case 14:09 Zeno` this leak doesn't even affect me (I restart every 24 hrs to make a backup) 14:09 celeron55 which would be best handled with time measurement like i suggested earlier (if the loop goes on for 60 seconds or something, just break and clear the queue so that the server can do other things) 14:09 Zeno` I'm not doing it for me 14:10 Zeno` if that loop goes for 60 seconds then... hmm 14:10 celeron55 it's 99% likely griefing in that case 14:10 celeron55 and very bad one 14:10 Zeno` I'm testing with minimal though 14:11 Zeno` and it's still not keeping up. I'm not even doing anything, just connecting the client 14:11 Zeno` perhaps there should be a limit there, I agree 14:12 Zeno` 60 seconds is too long though 14:13 Zeno` anyway as it currently stands there is already a DoS 14:13 Zeno` because the server runs out of RAM 14:15 celeron55 get the smoothing to actually work; then it's as good as before but with automatic scaling of loop_max 14:15 Zeno` perhaps the default for liquid_loop_max needs to be increased as well 14:15 celeron55 for smoothing to work the limit shouldn't increase in 10 steps by more than 2-3x or so 14:16 Zeno` ok, well the fix to be included with my patch is to increase liquid_loop_max 14:16 Zeno` you cannot smooth forever without dropping the bottom out somewhere 14:17 celeron55 don't hide the problem with it though; the key here is to make it work even with a grossly wrong liquid_loop_max value (that's the original problem anyway - with a large enough value this problem does not exist) 14:17 Zeno` but the problem with re-emerge once the smoothing becomes too much 14:17 Zeno` won't it? 14:17 celeron55 Zeno`: smoothing is intended for cases where something puts a million nodes in the queue and then puts zero nodes in the queue for minutes 14:17 kahrl well I'm working on optimizing the inner loops of transformLiquids 14:18 kahrl which would allow a larger liquid_loop_max of course 14:18 Zeno` celeron55, I understand that, but currently the DoS is still there 14:18 Zeno` because the server will crash OOM 14:18 kahrl (need to leave now though, cya) 14:18 celeron55 in denial-of-service attacks you have to stop giving service; i.e. drop the queue 14:18 celeron55 there are no options 14:19 Zeno` that does not currently happen 14:19 celeron55 yes it does not; it should 14:19 Zeno` well it doesn't 14:19 Zeno` yeah 14:19 Zeno` sorry, misread 14:19 Zeno` there are two issues here then 14:20 Zeno` I've fixed the first, where RAM can overflow 14:21 Zeno` maybe someone else can address the 2nd. I'm tired celeron55... been working on this issue that I don't care much about for 6 hours now (those massif tests take a while) :( 14:21 Zeno` Or maybe I can work on it tomorrow 14:21 Zeno` I dunno. I just want the leak fixed for now 14:28 Zeno` maybe if m_transforming_liquid.size() is > u32_max / 3 * 4 then the whole queue can be dumped. I'd rather the server crashed though if it got that out of hand 14:29 celeron55_ but that's then exactly what the griefer wanted, if it is a griefer 14:30 Zeno` like I said... I am fixing the leak, not addressing a DoS (not downplaying that importance either) 14:30 shadowzone buh bai 14:30 shadowzone Wowie 14:30 Zeno` at the moment the person doesn't even have to be a griefer 14:30 celeron55_ well just get the scaling-smoothing right, no need for DoS stuff now 14:30 Zeno` nobody has to be a griefer... the server will simply run out of RAM 14:31 Zeno` there are two issues here then 14:31 Zeno` * werwerwer (~1@158.181.242.132) has joined 14:31 Zeno` I've fixed the first, where RAM can overflow 14:31 Zeno` maybe someone else can address the 2nd. I'm tired celeron55... been working on this issue that I don't care much about for 6 hours now (those massif tests take a while) :( 14:31 celeron55_ no, your fix is bad if you only want that 14:31 celeron55_ just remove the whole loop_max if you only want a fix for RAM 14:31 celeron55_ it's just confusing code that is doing nothing useful otherwise 14:32 celeron55_ and yes, this is tiring but what i'm saying is a factt 14:32 celeron55_ -t 14:32 Zeno` My fix is good because it stops the server running out of RAM because of a leak that should not be there 14:32 celeron55_ let me show you a good fix for that 14:32 Zeno` And what I am saying is a fact as well 14:33 Zeno` Why would I remove the whole loop_max? 14:33 shadowzone Did I step into a discussion too late cause I'm lost. 14:33 shadowzone ? 14:33 celeron55_ https://gist.github.com/celeron55/8d222e3cb0c1fd7c58e6 14:33 celeron55_ this fixes the memory problem 14:34 celeron55_ much simpler and behaves essentially the same as your complicated one 14:34 Zeno` umm, no it doesn't 14:34 celeron55_ it's exactly the same in practice 14:34 Zeno` I can set liquid_loop_max = 50000 and I don't get a leak at all with or without my patch 14:34 celeron55_ yours tries to add smoothing, but fails completely at it 14:35 Zeno` ok, we'll leave the leak there 14:35 celeron55_ and that whole setting is useless anyway because it should be automatic 14:36 celeron55_ i can push this if someone agrees with this 14:36 Zeno` agrees with what? 14:37 Zeno` your gist? It doesn't limit 14:37 Zeno` mine patch... does limit 14:37 celeron55_ no it doesn't 14:37 Zeno` it limits to u32_max / 4 14:37 celeron55_ that's a meaninglessly large number 14:37 Zeno` suggest a lower number (power of 2) then 14:37 celeron55_ the server practically hangs way before that 14:38 celeron55_ limiting it that way enables the memory leak once again that you wanted to get rid of 14:38 celeron55_ this is the wrong way to go 14:39 celeron55_ maybe i'll just push my simple fix with some TODO comments 14:39 Zeno` umm, no 14:39 Zeno` because the goes away 14:39 Zeno` the leak goes away* 14:40 Zeno` your patch is broken 14:40 Zeno` it's not limited in any way 14:40 Zeno` for every single case apart from a DoS my patch fixes the issue 14:40 Zeno` yours fixes none 14:41 Zeno` and makes DoS even easier (quicker to perform) 14:42 Zeno` anyway, I'm going to sleep :) 14:50 celeron55_ https://github.com/minetest/minetest/pull/1981 14:58 Zeno` #1979 14:58 ShadowBot https://github.com/minetest/minetest/issues/1979 -- Do not allow the m_transforming_liquid queue to increase until all RAM is consumed by Zeno- 14:58 Zeno` happy now? 14:58 Zeno` it's better than your knee-jerk patch :/ 14:59 Zeno` http://pastebin.com/iMVg9yCM 14:59 celeron55_ what does it do different 14:59 celeron55_ it doesn't seem to be smoothing anything still 15:00 Zeno` it does 15:00 Zeno` if (m_transforming_liquid.size() < loop_max * 16) 15:00 celeron55_ i can only accept the complexity of your code if it does proper smoothing 15:00 Zeno` it does 15:00 Zeno` yours does no smoothing at all 15:01 celeron55_ your current one does so little smoothing it doesn't matter in practice 15:01 Zeno` mine adjusts liquid_loop_max if the server admin got it wrong 15:01 Zeno` :/ 15:01 Zeno` well, let's make it *4 then 15:02 celeron55_ also that paste shows very badly what it's doing 15:02 celeron55_ why is loopmax following tlgsz instantly? 15:02 celeron55_ it should stay largely still, that's what smoothing is all about 15:03 Zeno` http://pastebin.com/6iFD39U3 16 seems better, but *shrug* 15:03 Zeno` look at the code 15:03 Zeno` where the couts are located makes no difference 15:04 celeron55_ why is loopmax rising when it's already higher than tlqsz? 15:04 Zeno` it's not 15:05 celeron55_ loopmax initial=10000 tlqsz=10358 loopmax=20000 15:05 celeron55_ loopmax initial=10000 tlqsz=11473 loopmax=40000 15:05 celeron55_ loopmax initial=10000 tlqsz=11619 loopmax=80000 15:05 celeron55_ loopmax initial=10000 tlqsz=11100 loopmax=160000 15:05 celeron55_ at least two things are wrong in this 15:05 celeron55_ 1) it's rising for no reason, 2) it's rising too fast (2x is too fast) 15:06 Zeno` the paste is misleading. Look at where they're printed in the code 15:06 Zeno` 2x is not too fast to recover from a runaway situation 15:07 celeron55_ i'm now completely fed up with this and hope that other devs understand my point 15:07 Zeno` I doubt anybody knows what that point is :/ 15:09 Zeno` I'm not going to add an extra few variables so something I am paste-binning aligns with your thoughts (sorry) 15:09 Zeno` And to be honest I am fed up as well. I am *not* fixing this bug for the fucking fun of it 15:10 Zeno` And it IS a bug 15:10 Zeno` a fucking horrible bug 15:12 Zeno` And do you know why it's showing up now? 15:13 Zeno` Because I made all those getNode() functions at least twice as fast 15:13 Megaf celeron55_: Zeno`: I just dont understand why this leak comes from, it wasn't like that a couple of months ago 15:14 Megaf That's new 15:14 Zeno` it started when getNode became too fast for the flowing stuff to keep up with 15:15 Megaf can't it get slowed down again? 15:16 Zeno` you want the whole game slowed down again? 15:16 Zeno` that's easy, I'll just revert the getNode() optimisations 15:17 Megaf no, I want my server to not crash because it used all memory 15:17 Zeno` *shrug* my fix does that 15:18 Megaf https://github.com/minetest/minetest/issues/1880#issuecomment-67055770 15:18 Megaf #1880 15:18 ShadowBot https://github.com/minetest/minetest/issues/1880 -- Server memory usage slowly rises and doesnt stop 15:18 Megaf did you see that? 15:19 Zeno` umm, yeah. I fixed the big leak last week and the smaller one today (well, the one today cannot be accepted yet) 15:20 Zeno` celeron55_ would rather get rid of the "smoothing" all together for some reason 15:37 VanessaE Megaf: you use OSX right? 15:40 VanessaE celeron55_: recall that pilzadam and I tried to work out some kind of slow-down-the-DoS-potential to that liquid flowing code a while back (months ago), and it involved fucking around with the loop-max settings; he did the code though, I just provided the test bed and some suggestions that seemed to at least work for my system at the time. 15:41 VanessaE at the time, it didn't cause what either of us called a memory leak, but it did cause liquid reflowing to get "queued up" too much if there was a lot to process 15:42 VanessaE but I guess with other improvements in the engine of late, this needs revisited; go with zeno's code. 15:44 hmmmm so erm 15:44 hmmmm why isn't anybody checking how long liquid transforming takes 15:44 hmmmm not how many loops, how long 15:45 VanessaE nobody can be bothered? 15:45 VanessaE that was suggested in the argument earlier from what I saw 15:45 VanessaE why nobody coded it, idk. 15:45 VanessaE and I distinctly recall suggesting the same when pilz and I were discussing this originally. 15:45 VanessaE but I don't remember why it wasn't coded then, either. 15:47 VanessaE cue someone suggesting this has already been fixed in fm :P 15:48 Zeno` I can add another layer of code to time how long it takes 15:48 Zeno` but that will complicate it even more 15:48 Zeno` my PR fixes (for probably 99% of cases) the memory leak 15:50 Zeno` besides... how long it takes is only 1 part of the equation which is why I said earlier (maybe 2 hours ago) this is 2 separate issues 15:51 Zeno` size is most important; how long it takes to process a queue of size x is another issue 15:52 Zeno` how long it takes to process it not the most important because knowing it doesn't do much to allow the queue size to shrink 15:54 Zeno` the thing that is needed is to: a) shrink the queue size as quickly as possible when it's above a threshold; and b) still limit that "go for broke as quickly as possible" shrinkage 15:54 VanessaE Zeno`: I would recommend a specific test though... 15:54 VanessaE (see /msg) 15:55 celeron55_ hmmmm: ? kahrl started to optimize it, and i already suggested to measure time instead of loops 15:56 celeron55_ my PR basically removes the broken loop limit (and thus the infinite queue size causing problems) and allows building a working system in its place 15:56 celeron55_ (well, that's what i wrote to the description...) 15:57 Zeno` celeron55_, your PR introduces an infinite queue size! 15:58 Zeno` no, that's not right 15:58 Zeno` it causes a loop that is open to DoS 15:59 Zeno` and a much more rapid DoS than could possibly be caused by the current code or my patch 15:59 Zeno` celeron55_, what's the address of your server? 15:59 Zeno` I'd like to test some things 15:59 Zeno` (address and port # of course) 16:03 celeron55_ your patch makes DoS take 2 seconds instead of 0 16:04 celeron55_ i.e. no practical difference 16:04 Zeno` try it 16:04 Zeno` show me the numbers 16:04 Zeno` I have other tests I'd like to try on your server as well 16:05 Zeno` What I'm trying to say is that if you cannot show me real life numbers (hard data) then you have nothing to contribute to this particular discussion 16:06 Zeno` So, please, show me the data 16:20 neoascetic What you developers think on this comment? https://github.com/minetest/minetest/issues/1894#issuecomment-66921052 16:21 neoascetic / 16:21 neoascetic I also think these damage types [lava/drowing] should be applied to all entities with fleshy armor group, otherwise every mobs mod developer must handle this stuff by its own. 16:31 Calinou neoascetic, now implement it ;) 16:31 Calinou this is far from simple 16:31 Calinou this could be done in builtin Lua 16:32 neoascetic why in lua? 17:08 neoascetic there is $8 bounty for issue from me btw :D 17:15 Calinou that's not going to attract anyone :P 17:16 Calinou the feature will either imply a performance decrease, or complicated coding 17:17 neoascetic currenly this "complicated coding" is on shoulders of mods developers 17:17 Calinou so you can implement it in all entities, which will make everything slow :P 17:18 Zeno` I don't think slow is an issue 17:18 Calinou server-side, it is 17:18 Calinou people host servers on VPSes 17:20 neoascetic lua code is executed on server-side anyway... am I right? 17:21 Calinou yes 17:21 Calinou so you want to keep the server as fast as possible, avoid hacks 17:34 neoascetic So algorithmically there is no difference where the code executed: at level of engine or at lua level 17:35 neoascetic Even more, if each mobs module will not implement similar code, it will even be faster, probably 17:35 Calinou both would be server-side, but of course, something done in C++ will be faster. 23:21 grondilu Hello, I got a segfault when trying to run the latest dev branch. No more segfault after removing my minetest.conf file. 23:26 hmmmm oh, okay 23:29 grondilu also, just created a new single-player world, and could not see anything after spawning. I then realized I had spawned inside a tree or something. I had to punch my way out. 23:33 grondilu (FYI: debian GNU/linux, i586) 23:34 hmmmm ..thanks 23:35 hmmmm maybe it could be a little bit more helpful if you posted the config file that actually made the segfault, or a backtrace, or both 23:40 grondilu yeah, next time it happens, I will. But since I removed the file, I can't now.