Memories

If there's one thing science has taught me, it's that the best way to sell an idea is with pretty pictures. So, here's a pretty picture:


We've been having memory issues in the game intermittently since we came back, and the diagram offers a good illustration of the problem. When this bug first manifested, it so happened that I was trying to edit something on our game server and was rudely surprised by all manner of warnings about having insufficient memory to even open the text editor. Sure enough, somehow our game's memory footprint had blown up (seemingly overnight) to over twice its normal size, and there was very little to indicate what had happened. The solution then was to immediately reboot the MUD to flush memory and start from scratch, contact our provider and up our memory, and create a tool to keep track of our game's memory usage.

I've been checking the data dumped out by this tool daily to see if the memory blowup could be reproduced, and today was the first day I had a bite on the line. I suspected to find logs of someone doing something stupid coinciding with this big spike (like spamming haggle, exploiting some bug, etc), but there was nothing. Going back to my own personal logs of that morning though, the memory spikes happened around a time when Sidonie and I were working out some problems with a builder command. Ah-ha!

As it turns out, a very useful but not-often-used command that was put into the code many years ago had a very severe memory leak in it. The command uses the regular expression engine to parse boatloads of mobprogs iteratively, but the code wasn't cleaning up the tail end of the regex calls because the regex libraries silently malloc huge blocks of memory that aren't automatically freed when the parent routine exits.

This was a pretty easy mistake to make; while most high-level programming languages in which one would utilize regexes (like Perl, PHP, and whatever else) abstract away all of the memory management, C certainly does not. I guess the pitfall here is that the regcomp() call buried mallocs within it, so a headfirst dive into using regexes in C wouldn't directly expose the programmer to the mallocs which would raise alarms. An easy pitfall, sure, but like a wise man once said, "if you are going to write C code, you better be willing to deal with the memory management."

For what it's worth, I've gotten the game's code to be remarkably scarce on memory leaks like this despite the fact that I (nor any past coder of whom I know) has ever profiled the code with something like Valgrind or even gprof. While this case may give a pretty good argument for trying to figure out a realistic way to do this, blind dependence on tools like Valgrind to make up for incompetent coding has been known to cause serious problems. If we start running into memory leaks again though, maybe I'll try to figure out a way to set up profiling on a test port somewhere and have some of you players beat on it for a night.

PK Poll

A lot of people have been vocal about the perceived imbalance in our current PK code, so in the interests of getting a lot of one-click feedback, I've opened a poll (should appear to the right) regarding the matter. Please leave comments below.

April 5, 2011: The results are in!

• 62% felt that spellcasters need a boost
• 34% don't PK
• 31% felt it seems pretty balanced
• 21% think fighters need to be toned down
• 18% feel fear is too powerful
• 12% think guild spells make fighters too strong

This was out of a total of 32 votes.

Although we have no immediate plans to change the fighter/spellcaster dynamic (we have a lot of other changes in the works that need to be finalized before then), we will revisit these results and the comments posted and see if we can adjust accordingly.

Crying and complaining about coding

One of the hardest parts of coding for Dark Risings is the fact that it's derived from code that is over two decades old (Diku was first released in 1990) and, even after transforming into the ROM 2.4b6 codebase (released in 1998), it's been hacked on by twelve years of Dark Risings coders. As much as I hate to say it, the greatest amount of damage to the code has, in fact, been done by the long history of DR coders of varying levels of familiarity with both C and how ROM works.

What's prompted this whiny rant is a problem I discovered while generating a list of all of the scrolls in the game for Sidonie. I'd already done this with wands and staves with the help of a little Perl magic that I wrote, so I expected it to be another straightforward five-minute job. In a sense it was, but upon examining my data dump, I noticed that some scrolls had four spells (some of which are listed as "reserved") and others have blanks where spells should've been. Thinking that my Perl script was defective, I checked the area files themselves, and sure enough, some scrolls have "reserved" and others are just "". What's the difference? Since this sort of ill-defined behavior can lead to major issues (area/pfile corruption, crashes, etc), I started digging and was brought to one particular routine that made me a little crazy.

Consider the following code, straight out of the Dark Risings source:

/*
 * Lookup a skill by name.
 */
int skill_lookup( const char *name )
{
  int sn;

    for ( sn = 0; sn < MAX_SKILL; sn++ )
    {

      if( skill_table[ sn ].name == NULL )
        break;

      if( strcasecmp( name, skill_table[ sn ].name ) == 0 )
        return sn;

      /*
        if ( skill_table[sn].name == NULL )
            break;
        if ( LOWER(name[0]) == LOWER(skill_table[sn].name[0])
        &&   !str_prefix( name, skill_table[sn].name ) )
            return sn;
      */
    }
...

For whatever reason, a past coder decided it best to comment out the stock bits of this subroutine, presumably to force the subroutine to match the whole skill/spell name instead of allowing abbreviation (which can cause issues when you have spells like "lightning bolt" versus "lightning breath").

This sort of thing drives me crazy for many reasons, but here's one.

The stock version of the code was smart; it first checked the first character of the argument against the first character of the entry under examination in the master skill table. Since there's a very large chance that those first characters won't match when ripping through all 298 skills, you wind up not having to incur the overhead associated with a full-on subroutine call for the majority of the misses. This makes the whole process of finding a skill and returning its associated skill number much faster.

Perhaps I am more mindful of the benefit of these tiny performance benefits since I write very computationally intensive scientific code, where one poorly written line of code can add days or weeks onto compute time, for a living. And perhaps there really is no appreciable speed benefit to not preserving this preliminary first-character check when matching strings on modern hardware. However, I see little reason to pull it out outright since it is a smart way of handling these sorts of lookups.

Furthermore, the str_prefix routine (provided with ROM) was replaced with strcasecmp, which is intrinsic to string.h. However, whoever did this code change must have been extremely unfamiliar with ROM's internals (or perhaps extremely tired), because ROM provides str_cmp, its own equivalent to strcasecmp which is used extensively (and I mean extensively) throughout the code.

If strcasecmp and str_cmp serve the same purpose, why do both exist? As it turns out, strcasecmp is indeed included in string.h, but it is not ANSI C (although it is POSIX 2001). POSIX didn't exist back when Diku was first released (and neither did GCC or glibc for that matter), so it's likely that the str_cmp routine was written into the code decades ago. Providing this routine made the code more portable by not having to rely on special extensions that only existed in specific proprietary compilers. It was also tailored specifically to the needs of the code.

For the sake of consistency and portability, str_cmp (which is probably a little faster than strcasecmp) has been used exclusively in ROM's source. Unfortunately, DR's code has become rife with a sloppy mix of strcasecmp and str_cmp, forfeiting the benefits of portability while gaining literally nothing. As DR coders have touched the code, they've left these marks across it without really considering what the implications may have been.

True, since strcasecmp is now POSIX, it is unlikely that Dark Risings will ever be run on a system without it (e.g., DR compiles on HP-UX 11i, which predates POSIX 2001), but this is only an innocuous example. This sort of messy work, where one feature is implemented many times because various coders were ignorant of their predecessors' work, are all over the DR source, and it drives me a little nuts. I could go through and change all those strcasecmps to str_cmp, but to do so would require quite a bit of testing and would open the doors to new bugs. When it comes down to spending time cleaning up code which will not introduce any new features to the game or spending time adding new features while leaving the existing problems as they are, I find myself always choosing the latter.

Nobody cares if our code reads like garbage as long as that garbage is transparent to the players and imms, so I guess I'll have to take out my frustrations on this blog. It probably doesn't help that I spend my entire day at work cleaning up garbage FORTRAN.