03:35:11 Experimental (bcrawl) branch on underhound.eu updated to: 0.23-a0-5261-gd9800d219b 08:24:36 New branch created: monster-cleanup-deferred (2 commits) 13https://github.com/crawl/crawl/tree/monster-cleanup-deferred 08:24:36 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-688-g2ecdf0eabb: Defer reset of monster slots 10(21 hours ago, 12 files, 87+ 34-) 13https://github.com/crawl/crawl/commit/2ecdf0eabb3c 08:24:36 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-689-g3b5614c5fd: Keep monsters in mid cache until reset 10(17 hours ago, 12 files, 28+ 68-) 13https://github.com/crawl/crawl/commit/3b5614c5fd32 08:31:40 <11Odds> ^ This seems to be working to keep monsters around until the end of the turn. It's a slightly scary one, since there are lots of strange flows here; I'd certainly appreciate another pair of eyes on it some time. 08:40:37 <04dracoomega> Looks good at a glance, but I will take a more thorough look at it when I'm more awake. (I think this is the kind of backend change where overlooking something is probably inevitable, but it seems like a much better basis to operate on into the future) 08:45:11 04Build failed for 08monster-cleanup-deferred @ 3b5614c5 06https://github.com/crawl/crawl/actions/runs/28954397892 08:48:23 <04dracoomega> Incidentally, while what you're doing in that regard makes perfect sense and I'm just being pedantic, the the dangling pointers must not survive comment makes me want to point out that I'm pretty sure the monster array remains in the same place in memory the whole time the game is running and monsters are simply reused. (But loading a new floor is already repurposing or resetting them as needed, of course.) 08:48:52 <04dracoomega> But I don't think it's possible for a pointer to a normal monster to ever dangle 08:49:18 <04dracoomega> (Just to point to a 'different' one) 08:50:01 <11Odds> Sorry yeah, that was me being imprecise. It means to say "we don't want to keep resets for the old levels monsters or strange things will happen" 08:50:58 <04dracoomega> Yeah, that's fine. (I just apparently felt compelled to point out what might be any misapprehension about how the code is structured ^^; ) 08:51:14 <04dracoomega> Which doesn't matter here, but maybe could some day; who knows? >.> 08:53:07 <11Odds> (I think the test failure is real but it's a test thing - I'm guessing tests blast through slots without taking turns in ways that wouldn't happen in real usage) 08:54:26 <04dracoomega> Seems plausible 08:55:02 <04dracoomega> (Dismiss monsters should reset on its own, I expect) 08:56:05 <04dracoomega> Probably worth having a careful look over other places that happen 'outside' of normal turns, where fineffs wouldn't be processed 08:56:33 <11Odds> Yeah, the one I did spot was building levels, which can place a lot of monsters to get their corpses 08:58:30 <11Odds> (It should be OK for a monster not to be reset if it dies at a funny time like wizmode dismissal; pending-reset monsters shouldn't have any adverse effects except occupying a slot) 08:58:41 <04dracoomega> Fortunately, at least a few places that make 'temporary' monsters don't do it in a way that requires them to ever be reset, since they never lived in env.mons 08:59:19 <04dracoomega> Well, what is happening here is that the env.mons is filling up with not-yet-reset monsters, which cannot be reused, and so it runs out of space to place new ones 08:59:38 <04dracoomega> So I think it's important that even wizmode commands don't lead to this behavior 09:00:09 <11Odds> Ah right I guess you could trigger this in wizmode by creating and destroying hundreds of monsters between turns 09:00:43 <11Odds> But as soon as you take a turn they'd all get cleaned up (though here I agree, dismissal should just reset) 09:01:21 <11Odds> The point is more that we shouldn't have to worry overly much about normal quantities of deaths at odd times in the turn cycle 09:03:06 <04dracoomega> I'm pretty sure that even 'instant' actions which the player can take already must be handling fineffs, or even weirder things would be happening 09:03:19 <04dracoomega> Outside of wizmode, I mean 09:04:19 I'm pretty sure I've even patched around one of them (check PRs submitted by me) 09:04:57 crawlcode is just filled with spooky action at a distance 09:05:05 <04dracoomega> (Like, Chei stuff effectively calls fire_final_effects() as monsters do things, for instance) 09:06:09 <11Odds> I think for this it would be fine even if we didn't do fineffs on some instant action 09:06:41 <11Odds> Monsters who are pending_reset shouldn't effect anything unless you already know about them or their mid 09:07:41 <02Monkooky> Can you break this by saving at a weird time? 09:08:04 <11Odds> Saving a level resets all the monsters, is the way I try to make the answer "no" 09:08:42 <04dracoomega> Yeah, that seems easy to make safe 09:09:17 <04dracoomega> Like, I think as a design principle, any dead monsters should not affect anything by the time the player has control again. It's only the 'inbetween' or 'mid-action' times 09:09:31 <04dracoomega> So it's fine to discard them at any time the player is involved 09:09:48 <04dracoomega> Er, 'the person behind the keyboard' rather than the player character, to be clear 09:09:50 <11Odds> (We also discard final effects at this time, with similar logic) 09:10:55 <11Odds> I did spot the comment // Shouldn't happen, but this is too unimportant to assert. for that (not having final effects pending on load), when I think it can in fact happen. 09:12:21 <04dracoomega> Under what circumstances? 09:12:33 <04dracoomega> Maybe something involving banishment 09:13:07 <04dracoomega> 'Take enough damage via attack to trigger some retaliation effect, then get banished before it goes off', maybe?? 09:13:38 <04dracoomega> Though it does sound mostly harmless to ignore any of that 09:13:59 <11Odds> I think Ignis shafting you was the case that looked like it moved you off-level outside the approved times 09:14:06 <04dracoomega> Perhaps 09:14:22 <11Odds> I didn't check super carefully and think it's probably fine 09:14:25 <04dracoomega> (I agree it does seem 'too unimportant' to do anything with but ignore) 09:14:58 <04dracoomega> Not even objectively incorrect in-world logic, even 09:17:54 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-690-g6ba7fa5ed9: fixup! Defer reset of monster slots 10(26 minutes ago, 1 file, 1+ 1-) 13https://github.com/crawl/crawl/commit/6ba7fa5ed918 09:17:54 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-691-g835a77d9b0: Reset monsters immediately on dismissal 10(22 minutes ago, 1 file, 4+ 1-) 13https://github.com/crawl/crawl/commit/835a77d9b03f 09:33:07 04Build failed for 08monster-cleanup-deferred @ 835a77d9 06https://github.com/crawl/crawl/actions/runs/28957995889 09:34:46 <04dracoomega> mid cache bogosity Oh no ^^; 09:35:08 <11Odds> Nah it's the test that's wrong 09:35:35 <11Odds> The mid cache doesn't return cleaned-up monsters by default, so it is not finding a monster it expects 09:36:30 <11Odds> (This seemed safer than auditing 100 mid cache users to check if they would cope with dead monsters coming back) 09:40:18 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-692-gdf59c66245: Don't leak monster slots in fsim 10(16 minutes ago, 1 file, 1+ 1-) 13https://github.com/crawl/crawl/commit/df59c6624558 09:40:18 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-693-gac17f3f8a3: Fix a debug scan 10(3 minutes ago, 1 file, 1+ 1-) 13https://github.com/crawl/crawl/commit/ac17f3f8a32a 09:42:59 <04dracoomega> Oh, huh 09:43:14 <04dracoomega> Okay, yes, makes sense 09:52:27 04Build failed for 08monster-cleanup-deferred @ ac17f3f8 06https://github.com/crawl/crawl/actions/runs/28959449078 09:53:48 <11Odds> Hmmmm.... now it really shouldn't be happening. 10:02:21 <04dracoomega> Incidentally, you do know you can run those test suites locally, right? 10:02:47 <04dracoomega> (The tag upgrade stuff is a bit of a pain, but the standard test suite is easy) 10:03:35 <11Odds> Yeah, I do, was just overconfident I knew what was up 10:06:00 <04dracoomega> That is fair and reasonable ^^; 11:28:01 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-694-gdccef52f31: Fix a mid-cache problem with transiting monsters 10(8 minutes ago, 1 file, 8+ 1-) 13https://github.com/crawl/crawl/commit/dccef52f31e1 11:28:57 <04dracoomega> @Odds I'm not certain I understand the issue that commit is trying to address. At first brush, it feels like a different bug if the same mid ever refers to more than one monster at any point. They're supposed to be completely unique IDs. Am I missing something? 11:30:40 <11Odds> I'm pretty sure what is happening is: - Abyss shifts - Old copy of monster gets killed (_abyss_lose_monster) but not reset - New copy of monster gets created, with the same MID, takes over cache entry - (before this commit) old copy of monster clears that cache entry 11:32:01 <11Odds> (These are monsters who get displaced by walls appearing when they were and such, I think? abyss_morph is where I think this stuff is happening) 11:34:38 <04dracoomega> Huh. Abyss shifts can preserve monsters? 11:34:54 <04dracoomega> I thought once something got lost in the Abyss, it was generally just lost for good. 11:35:30 <11Odds> If they needs_abyss_transit, which means they are unique or banished by the look of it? 11:36:12 <04dracoomega> (And like, experimentally, when I was looking into TRJ banishment after the slimy stairs change, I found that once they were placed in the Abyss a single time, abyss shifts seemed to prevent them ever appearing again) 11:36:28 <04dracoomega> Like, perhaps that is not 100% true, but it sure seemed like it to me at the time 11:38:21 <04dracoomega> Not sure if there are different circumstanes involved somehow 11:39:10 <11Odds> Yeah I'm not especially up to speed on the details of this stuff 11:39:46 <11Odds> It certainly looks like _abyss_lose_monster is aiming to preserve some monsters 11:42:14 04Build failed for 08monster-cleanup-deferred @ dccef52f 06https://github.com/crawl/crawl/actions/runs/28966114268 12:13:55 03CrawlOdds02 07[monster-cleanup-deferred] * 0.35-a0-695-gc627e9024f: Don't apply enchantments posthumously 10(2 minutes ago, 1 file, 4+ 0-) 13https://github.com/crawl/crawl/commit/c627e9024f83 12:14:15 <11Odds> That one (which fixes the last build failure) is an especially sneaky bug which I would never have caught without the tests - the apply_enchantments loop was relying on monster cleanup to break out if the monster died while applying enchantments 12:14:33 <04dracoomega> Huh 12:14:40 <04dracoomega> To like... poison and such things? 12:14:54 <11Odds> Or to summonedness decaying I think 12:15:10 <04dracoomega> Sure, yeah 12:15:21 <04dracoomega> A fair number of way to die in that function, really 12:15:41 <11Odds> Yeah, and when the monster did the function used to exit in a super indirect way 12:16:09 <04dracoomega> "It effectively never caused a problem before, so nobody had to think about it." 15:43:02 Unstable branch on underhound.eu updated to: 0.35-a0-688-gec49406eb0 (34) 18:36:52 <04dracoomega> @gammafunk I've been trying queries like !lm * recent br.end=Orc x=avg(xl) on a couple days lately and they always seem to fail due to timeout. Is there something about this which newSequell really doesn't like or am I doing something wrong? 18:45:30 that's been going on since it came online, no? 19:57:23 <09gammafunk> a query like that is perfectly reasonable, and I think newsequell is just having trouble processing the sheer number of rows in the milestones table (even allowing that you're conditioning on version) 19:57:48 <09gammafunk> there is some weirdness I've noticed when version numbers are involved (among other fields) where it seems to be faster when you wouldn't expect 19:57:51 <09gammafunk> !kw recent 19:57:53 <04Cerebot> Keyword: recent => cv>=0.33 19:58:20 <09gammafunk> !lm * current br.end=orc x=avg(xl) 20:01:21 <04Cerebot> 180s limit exceeded: killed !lm * current br.end=orc x=avg(xl) 20:02:56 <09gammafunk> I do have some longterm ideas to try to improve things, but they involve taking sequell down, making backups, and trying a postgres upgrade along with some settings changes 20:04:11 <09gammafunk> I'm waiting to see how long this current query is going to take 20:04:43 <09gammafunk> there we go 20:07:23 <09gammafunk> ah but it's only the first part of the query and it too 380s 20:09:22 <09gammafunk> So aside from eventually taking sequell down for a while in order to work on it, which I don't want to commit to quite yet as I need to do a bit more research and have more time, one thing I might do is look into giving certain people permissions to run commands without a timeout 20:10:13 <09gammafunk> this is not a great solution in that it could take 10+ minutes to run certain queries, but it's a nice longterm solution to slow queries in general, and it allows a trusted, limited number of people to get access to potentially useful query data 20:14:00 <09gammafunk> all told it looks like the full query that I did just for current would have completed in a bit over 720 seconds 20:14:12 <09gammafunk> presumablye recent would be longer than that, although not sure by how much 20:15:08 <09gammafunk> !lm * cv=0.33 br.end=orc x=avg(xl) 20:15:29 <09gammafunk> I wonder if use of date helps here, I should play around with this 20:18:09 <04Cerebot> 180s limit exceeded: killed !lm * cv=0.33 br.end=orc x=avg(xl) 23:47:11 Monster database of master branch on crawl.develz.org updated to: 0.35-a0-688-gec49406eb0