fabiangreffrath / crispy-doom

Crispy Doom is a limit-removing enhanced-resolution Doom source port based on Chocolate Doom.
https://fabiangreffrath.github.io/crispy-homepage
GNU General Public License v2.0
800 stars 130 forks source link

Print minimal / allocated RAM in megabytes #427

Closed JNechaevsky closed 5 years ago

JNechaevsky commented 5 years ago

Could be interesting for console users, to see how much megabytes are used, not a magical nimber like 00000000666.

This line: https://github.com/fabiangreffrath/crispy-doom/blob/master/src/i_system.c#L165

Can it be something like reverse calculation a-la size / 1024 / 1024? Can't check right now.

Suggestion №108

JNechaevsky commented 5 years ago

Second half:

    printf("zone memory: %p, %i MB allocated for zone\n", 
        zonemem, (*size / 1024 / 1024));

We need %i format for a decimal integer, and divide it twice to get a megabytes value (first division = kilobytes, second = megabytes).

%p is zonemem, which is a byte in pointer address format in print. Hmph. Confusing.

JNechaevsky commented 5 years ago

For first half, instead of creepy number, we can indicate total amount of available RAM. Possible approach:

int pitto = SDL_GetSystemRAM();

    printf("zone memory: %i MB available, %i MB allocated for zone\n", 
        pitto, (*size / 1024 / 1024));

Result: image

But no, I don't like the idea of having four characters to indicate more than Gig memory. It should be something like:

Keep working...

JNechaevsky commented 5 years ago

Guess it's done:

    int total_ram = SDL_GetSystemRAM();
    char *gb = "GB";
    char *mb = "MB";

    printf("zone memory: %i %s available, %i MB allocated for zone\n",
           // %i - indicate total RAM available
           total_ram > 1024 ? // more than 1 GB?
           total_ram / 1000 : // print in GBs (8 GB)
           total_ram,         // print in MBs (512 MB)
           // %s - print GB or MB, depending of RAM available
           total_ram > 1024 ? gb : mb,
           // %i - print allocated RAM in megabytes (only)
           *size / 1024 / 1024);

image

Not sure if it can be simplified. Also note, that we need to divide by 1000 in GiGs, otherwise we'll get a bit wrong value (in my case: 7 instead of 8 GiGs of RAM). @fabiangreffrath what to you think?

WSSDude commented 5 years ago

char* with string literal would emit a warning for sure. char const* should be used instead.

The way you do it right now would lead to two comparisons and also doesn't look quite as good. Also, you could simply print G or M character as B is in both.

I would maybe do this instead:

int total_ram_mb = SDL_GetSystemRAM();
int total_ram_gb = total_ram_mb / 1024;
char size_char[2] = { 'G', 'M' };

printf("zone memory: %i %cB available, %i MB allocated for zone\n", 
  // %i - indicate total RAM available
  (total_ram_gb ? total_ram_gb : total_ram_mb), 
  // %c - print GB or MB, depending of RAM available
  size_char[!total_ram_gb], 
  // %i - print allocated RAM in megabytes (only)
  *size / 1024 / 1024
);

Much easier to read IMO, single comparison and no redundant ops (I believe at least).

Even shorter:

int total_ram_mb = SDL_GetSystemRAM();
int total_ram_gb = total_ram_mb / 1024;

printf("zone memory: %i %cB available, %i MB allocated for zone\n", 
  // %i - indicate total RAM available
  (total_ram_gb ? total_ram_gb : total_ram_mb), 
  // %c - print GB or MB, depending of RAM available
  "GM"[!total_ram_gb], // <-- this is valid too and doesn't require extra variable, but also less readable
  // %i - print allocated RAM in megabytes (only)
  *size / 1024 / 1024
);
JNechaevsky commented 5 years ago

Looks awesome! @WSSDude, thanks for effort and explanations!

WSSDude commented 5 years ago

Just came to my mind that this could also be done, but I think at this point, it is just overthinking everything lol :D Also, it is not actually shorter but longer... Should maybe be better though performance-wise but dunno... It has no comparisons at all. This seems quite a much at this point IMO though :D

int total_ram[2]; // at index 0 in GB, at index 1 in MB
total_ram[1] = SDL_GetSystemRAM();
total_ram[0] = total_ram[1] / 1024;
int in_mb = !total_ram[0]; // if RAM size in GB is zero, use size in MB

printf("zone memory: %i %cB available, %i MB allocated for zone\n", 
  // %i - indicate total RAM available
  total_ram[in_mb], 
  // %c - print GB or MB, depending of RAM available
  "GM"[in_mb],
  // %i - print allocated RAM in megabytes (only)
  *size / 1024 / 1024
);

No problem btw :P I'd like to start contributing here more maybe, just need to finish few more important things first. You may see me here more in future :)

fabiangreffrath commented 5 years ago

Thanks for the 108th suggestion @JNechaevsky ! Human-understandable zone heap size is a nice idea, though dividing by 1024 results in kiB and MiB. But available RAM won't happen in Crispy, this isn't MSD.EXE. :wink: Also, it's sometimes good to know the heap zone base address for debugging.

fabiangreffrath commented 5 years ago

Regarding your little RAM printing game, how about this?

const char *const prefix[] = {"B", "kiB", "MiB", "GiB", "TiB"};
int i = 0, size = SDL_GetSystemRAM();

while (size >> 10)
{
    size >>= 10;
    i++;
}
printf("%d %s\n", size, prefix[i]);

The llittle downside is that it only counts full units, i.e. 4096 results in 4 kiB, whereas 4095 results in 3 kiB.

WSSDude commented 5 years ago

Nice one lol. Still the B could be stripped though, for that one less char to process inside printf :P (RIP empty string for the byte though)

WSSDude commented 5 years ago

If I understood correctly btw, SDL_GetSystemRAM() return size in MB. So that B and KiB entry is probably wrongly there.

Also, i could be stripped as well in this case, ℅ciB should be better, with character array instead of that.

Or even more simply "MGT"[i] and no variable needed there :)

fabiangreffrath commented 5 years ago

If I understood correctly btw, SDL_GetSystemRAM() return size in MB. So that B and KiB entry is probably wrongly there.

Yep, right. Just seen that when I asked myself how SDL would like to return more than 3 GiB of RAM in bytes as a 32-bit int. :grin:

WSSDude commented 5 years ago

I made a little edit, it could be even more simplified in that case as you may see.

Anyway nice little code :)

fabiangreffrath commented 5 years ago

Final version? :grin:

int i = 0, size = SDL_GetSystemRAM();

while (size >> 10)
{
    size >>= 10;
    i++;
}

printf("%d %ciB\n", size, "MGT"[i]);
WSSDude commented 5 years ago

Yep, looks good 👍

JNechaevsky commented 5 years ago

this isn't MSD.EXE

mem.exe /c /p comes to mind back from DOS era, but looks likes both programs has been removed from latest versions of Windows.

Okay, fine, now I'm diseased with an idea to run Crispy on some kind of data center! AFAIK, we are having some data centers (or probably "computing clusters") on my day job, and some institutional laboratories are using them for some scientific research. To get access to the cluster, a statement must be signed by the chief of department who made the request. I could ask my chief to sign such request, and probably he will, because of good sense of humor, but I'm afraid when data center guys will read something like "access is needed to check out how fine Doom source port is running", they probably ask our whole it department just one question: "Wha-- ?!". So it should never happen. This suck, life is unfair. :grinning:

fabiangreffrath commented 5 years ago

Okay, fine, now I'm diseased with an idea to run Crispy on some kind of data center!

The idea is tempting, but I am afraid you would be disappointed. Data centers generate their computing power by sheer number of CPU cores. But Doom is as single-threaded as you can get...

JNechaevsky commented 5 years ago

Ah, yes. And looks likes there is no simple way to make all available CPU cores active, "parallelization" (if I'm not mistaking) is not just a magical compiler option.

JNechaevsky commented 5 years ago

Hey, just got a couple of thoughs about further console output! But before posting as a suggestion, may I leave them just as few raw ideas?

  1. Indicate how much time startup takes: from memory allocation to ST_init (last printing string, but probably not last in initialization).
  2. Indicate how much time takes level loading (game of P_SetupLevel?).
  3. Indicate how much time takes BLOCKMAP recreation (not sure about this one).

But I'm not sure what kind of internal timer should be used - gametic probably? Since gametic is mostly unclear value, it can be converted to something like: mm : ss : ms by some kind of division / multiplying magic.

Or is it something really unnecessary, because loading of ultimately huge maps like Sunder MAP15 (which is mostly stress-testing for us, since it's not really playable because of Boom specifics) just just few milliseconds?

fabiangreffrath commented 5 years ago

gametic is unsuitable, because it is counted by the engine itself. That means, if the engine takes 10 seconds to load a map, it won't increase gametic during this time. I_GetTimeMS() might be a valid candidate for this.

JNechaevsky commented 5 years ago

Alright, guess now I have some plans on weekend - need to think what kind of timers will be interesting / helpful. Console is an awesome thing (even on Windows, sic!), but we definitely don't need to print every thing happened, otherwise this will lead to:

Безымянный

JNechaevsky commented 5 years ago

...from another hand, do we really need timers? Startup process is blazingly quick, a game of milliseconds will not make any sense. Also, startup process is not a benchmark thing.

Next, about level loading time. Same thing. Even loading a Sunder MAP15 takes literally a few tics. What timer can show there? Possible aspect of map optimization for reducing loading time? Not sure, it's probably a most big & detailed level existing, which is working fine enough (except it is using Boom-specifics).