| Time |
Nick |
Message |
| 00:14 |
|
diemartin joined #minetest-dev |
| 00:22 |
|
Robby joined #minetest-dev |
| 00:41 |
|
domtron joined #minetest-dev |
| 01:58 |
|
Zeno` joined #minetest-dev |
| 02:04 |
|
paramat joined #minetest-dev |
| 02:05 |
paramat |
thanks for exposing chunksize on mapgen init, this will make lua mapgens a little simpler and faster |
| 02:09 |
|
proller joined #minetest-dev |
| 03:43 |
|
proller joined #minetest-dev |
| 04:18 |
|
paramat left #minetest-dev |
| 04:43 |
|
Megaf joined #minetest-dev |
| 04:55 |
|
electrodude512 joined #minetest-dev |
| 05:14 |
|
Wuzzy joined #minetest-dev |
| 05:18 |
|
Megaf joined #minetest-dev |
| 05:23 |
|
Wayward_One joined #minetest-dev |
| 05:29 |
|
Megaf joined #minetest-dev |
| 05:31 |
|
sol_invictus joined #minetest-dev |
| 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 |
| 05:52 |
|
Fritigern joined #minetest-dev |
| 05:55 |
|
msanchez_afk joined #minetest-dev |
| 06:06 |
|
daswort joined #minetest-dev |
| 06:15 |
|
MinerDad joined #minetest-dev |
| 06:19 |
|
Megaf joined #minetest-dev |
| 06:29 |
|
Megaf joined #minetest-dev |
| 06:31 |
|
neoascetic joined #minetest-dev |
| 06:51 |
|
Hunterz joined #minetest-dev |
| 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:55 |
|
jin_xi joined #minetest-dev |
| 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 |
|
DuDraig joined #minetest-dev |
| 08:43 |
Zeno` |
ok, I need help with it |
| 08:44 |
|
DFeniks joined #minetest-dev |
| 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:12 |
|
NakedFury joined #minetest-dev |
| 09:14 |
|
Amaz joined #minetest-dev |
| 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:43 |
|
neoascetic joined #minetest-dev |
| 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 |
|
DFeniks joined #minetest-dev |
| 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 joined #minetest-dev |
| 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:25 |
|
DFeniks joined #minetest-dev |
| 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:26 |
|
pro joined #minetest-dev |
| 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:05 |
|
deltib joined #minetest-dev |
| 12:08 |
|
kilbith joined #minetest-dev |
| 12:09 |
|
selat joined #minetest-dev |
| 12:11 |
|
pro__ joined #minetest-dev |
| 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 |
|
Amaz joined #minetest-dev |
| 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 |
|
kilbith joined #minetest-dev |
| 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:34 |
|
OldCoder joined #minetest-dev |
| 12:34 |
|
Taoki joined #minetest-dev |
| 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:14 |
|
ImQ009 joined #minetest-dev |
| 13:14 |
|
neoascetic joined #minetest-dev |
| 13:14 |
|
neoascetic joined #minetest-dev |
| 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 |
|
msanchez joined #minetest-dev |
| 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:32 |
|
msanchez left #minetest-dev |
| 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:38 |
|
DFeniks joined #minetest-dev |
| 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:42 |
|
ImQ009 joined #minetest-dev |
| 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 |
|
Wayward_One joined #minetest-dev |
| 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:49 |
|
exio4 joined #minetest-dev |
| 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<Value>::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 |
|
shadowzone joined #minetest-dev |
| 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 |
|
Wayward_One joined #minetest-dev |
| 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 |
|
Weedy joined #minetest-dev |
| 14:19 |
|
Weedy joined #minetest-dev |
| 14:19 |
Zeno` |
there are two issues here then |
| 14:20 |
|
werwerwer joined #minetest-dev |
| 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:22 |
|
ImQ009 joined #minetest-dev |
| 14:23 |
|
celeron55_ joined #minetest-dev |
| 14:25 |
|
sol_invi1tus joined #minetest-dev |
| 14:26 |
|
Amaz1 joined #minetest-dev |
| 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` |
<Zeno`> there are two issues here then |
| 14:31 |
Zeno` |
* werwerwer (~1 158.181.242.132) has joined |
| 14:31 |
Zeno` |
<Zeno`> I've fixed the first, where RAM can overflow |
| 14:31 |
Zeno` |
<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 |
|
CraigyDavi joined #minetest-dev |
| 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 |
|
n4x joined #minetest-dev |
| 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:35 |
|
Amaz left #minetest-dev |
| 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:43 |
|
proller joined #minetest-dev |
| 14:50 |
celeron55_ |
https://github.com/minetest/minetest/pull/1981 |
| 14:53 |
|
hmmmm joined #minetest-dev |
| 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:28 |
|
Hunterz joined #minetest-dev |
| 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:52 |
|
Eivel_ joined #minetest-dev |
| 15:53 |
|
MinerDad joined #minetest-dev |
| 15:53 |
|
roniz joined #minetest-dev |
| 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 |
|
proller joined #minetest-dev |
| 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:15 |
|
Calinou joined #minetest-dev |
| 16:19 |
|
neoascetic joined #minetest-dev |
| 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:24 |
|
kaeza joined #minetest-dev |
| 16:29 |
|
Eivel_ joined #minetest-dev |
| 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? |
| 16:33 |
|
Eivel_ joined #minetest-dev |
| 16:34 |
|
Amaz joined #minetest-dev |
| 16:44 |
|
DFeniks joined #minetest-dev |
| 16:47 |
|
Amaz left #minetest-dev |
| 16:53 |
|
electrodude512 joined #minetest-dev |
| 16:53 |
|
Eivel_ joined #minetest-dev |
| 16:58 |
|
MinerDad joined #minetest-dev |
| 16:59 |
|
troller joined #minetest-dev |
| 16:59 |
|
n4x joined #minetest-dev |
| 17:00 |
|
exio4 joined #minetest-dev |
| 17:08 |
neoascetic |
there is $8 bounty for issue from me btw :D |
| 17:10 |
|
Eivel_ joined #minetest-dev |
| 17:13 |
|
proller joined #minetest-dev |
| 17:15 |
Calinou |
that's not going to attract anyone :P |
| 17:15 |
|
pro__ joined #minetest-dev |
| 17:16 |
Calinou |
the feature will either imply a performance decrease, or complicated coding |
| 17:16 |
|
Eivel joined #minetest-dev |
| 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 |
|
rubenwardy joined #minetest-dev |
| 17:21 |
Calinou |
so you want to keep the server as fast as possible, avoid hacks |
| 17:23 |
|
Zeno` joined #minetest-dev |
| 17:28 |
|
Eivel joined #minetest-dev |
| 17:29 |
|
Hunterz joined #minetest-dev |
| 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. |
| 17:39 |
|
proller joined #minetest-dev |
| 17:40 |
|
Weedy joined #minetest-dev |
| 17:40 |
|
Weedy joined #minetest-dev |
| 18:11 |
|
shadowzone joined #minetest-dev |
| 18:14 |
|
shadowzone joined #minetest-dev |
| 18:19 |
|
MinetestForFun joined #minetest-dev |
| 18:22 |
|
Krock joined #minetest-dev |
| 18:24 |
|
electrodude512 joined #minetest-dev |
| 18:40 |
|
neoascetic joined #minetest-dev |
| 18:50 |
|
MinetestForFun joined #minetest-dev |
| 18:56 |
|
GrimKriegor joined #minetest-dev |
| 19:10 |
|
MinerDad joined #minetest-dev |
| 19:11 |
|
exio4 joined #minetest-dev |
| 19:12 |
|
n4x joined #minetest-dev |
| 19:38 |
|
roniz joined #minetest-dev |
| 19:39 |
|
neoascetic joined #minetest-dev |
| 20:03 |
|
FR^2 joined #minetest-dev |
| 20:27 |
|
shadowzone joined #minetest-dev |
| 20:45 |
|
roniz_ joined #minetest-dev |
| 20:54 |
|
AnotherBrick joined #minetest-dev |
| 21:40 |
|
exio4 joined #minetest-dev |
| 22:05 |
|
kilbith joined #minetest-dev |
| 22:10 |
|
electrodude512 joined #minetest-dev |
| 22:36 |
|
roniz joined #minetest-dev |
| 22:57 |
|
exio4 joined #minetest-dev |
| 23:05 |
|
paramat joined #minetest-dev |
| 23:20 |
|
grondilu joined #minetest-dev |
| 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. |
| 23:52 |
|
proller joined #minetest-dev |
| 23:58 |
|
electrodude512 joined #minetest-dev |
