GHOP #67: Shows how much memory is in use versus your server configuration's maximum memory

I wrote an article a while back on measuring memory consumption by Drupal bootstrap and modules.

It would be really nice if you can take the data collection part of that patch, and integrated it into devel, so there is a new setting in it to show memory consumption for every module.

subscribing

subscribing

I've written a patch for this problem but it needs some review. I'm not sure I put everything at the right place because I'm new to Drupal. I've attached my patch and a screenshot of the result.

The patch inserts some code into system.module that shows the memory info over the modules grid and also prints a warning if the memory usage is too high.

I've also written some code into system.admin.inc that adds a new column into the addon grid called "Memory usage". If a module gives information about it's expected memory usage this info is written here. The parameter in the info file is called "memory".

Subscribing.

I've enhanced my patch now. The changes:
- The rate_memory function is now called system_memory_info and is stored in system.admin.inc
- I've extracted the HTML and put it into theme_system_modules
- The estimated memory usage of the single modules is now shown as a popup if you move the mouse over the description

I finally found a way to get this JavaScript running :)

These patches required a lot of tinkering and some changes in several files. I'm again not sure I put everything in the right position.
I've tested the JavaScript only on Konqueror, so someone should test them on other Browsers if you think this feature is good.

This solution is not as clean as the one before in my opinion, but I don't know how to make better.

The estimated memory usage for the different modules have to be entered in their .info file (f.ex. 'memory = 0.18' means, that this module will use 0.18 MB of memory when it's activated).

You have to put the memorywarn.js file into the misc folder.

PhalanX

It is common practice to have all the changes in one patch, not one for each file changed.

See patching for details.

Okay, thank you for this link, I didn't know how to patch all files at once.
I've corrected it in my post above.

No time to actually test the patch today, but merely reading the patch file in #8 revealed some problems:

- I guess that $file->info['memory'] is supposed to be a number? Then, t() should not be used. t() is for making strings translatable for localization, so no need to make translators deal with numbers (also not very good to run t() on variables, due to automated translation-template extractor not finding actual strings).

- A bit later, t('<div id="meminf" class=" .... should be changed too: t() should only contain the actual text (whole text, including any variable data replacements INSIDE that text, to allow translators to change order), but not all the HTML (no need to present that to the translator). So, the line should probably look like this (unless some other changes needed for themability, which I don't see as necessary though, given that this is just one administrative page):
$output .= '<div id="meminf" class="messages '. $meminf['state'] .'">'. t('Memory usage: @mem_in_use of @mem_limit MB.', array('@mem_in_use' => $meminf['in_use'], '@mem_limit' => $meminf['limit'])) .'</div>';

- function system_memory_info() { should not have a linebreak before the curly brace.

- If $form['memory'][$key] is just a numeric value defined in the form data (as the code suggests to me), then no need for drupal_render()

- if (strlen($memory_info) > 0) { may be simplified to if (!empty($memory_info)) { unless I misunderstood something.

- Just below, 'Estimated memory required: ' needs a t(), otherwise the string will be untranslatable English on localized sites.

- I didn't look into the JS file (not much knowledge of JS), but if that's a system module specific file, it might live inside the modules/system directory?

Let me say again, that I didn't do any in-depth analysis yet, so these ideas are merely quickly-spotted pointers where to look, maybe. In general, the idea behind this patch is really nice, and I would really like to see this functionality in core. Good work :)

Thank you for your long response.

This strlen thing was the only thing that I got running in the beta3 version. I don't know why. Now I saw that this (of course much smarter) version works now.

I've corrected all the things you mentioned. When I started working on the dev version I discovered the possibility to place the JS files into other folders, so also my first dev-patch uses this method.

If you want to test this patch you have to put the JS file I attached in one of my last posts into the modules/system folder.
You can also add the memory line to one of the info files to test the JavaScript warning and the memory usage popup.

Now I'm really out-of-time, but one very quick comment: While adding new files, I always used diff -urpN instead of just diff -urp, so that new files are also rolled inside the patch. But please double-check, that no other files got rolled-in accidentally (such as automated back-up files made by some text editors and such).

EDIT: Also note the cosmetic spaces in concantenations, per Drupal's coding standards: The dot sticks to the strings, but should be separated by one space from variables, function calls and the like.
http://drupal.org/coding-standards
http://drupal.org/node/30785

Okay, I've added the spaces and included the JS file into the patch.
I'd like to know if there is something to improve furthermore or if it's ready to become implemented into the CVS?

Hi Phalanx,

I applied your last (complete) patch, no other patches applied. It is working as far as it displays the used memory:
> Memory usage: 8.42 of 128 MB. <

I have only the core installed, so no extra modules. On windows Vista, with FireFox 2.0.0.11 and Microsoft Explorer 7.0 i don't see any update in the memory usage field. When I use Firebug, and set a breakpoint, the function updateMem is only reached on loading of the page, NOT when I click on the checkbox for "Book".

Don't know if this is a fault?

Edit: Sorry, was a fault om my side. I am not that good in FireBug yet. Found out that upadateMem is called, but "Book" doesn't give a MemUse (yet). So updateMem returns without update.

Frans

Have you added a line for the memory in the info file of the book module as I described before?

Otherwise my patch won't know what's exactly the memory usage of this plugin and so will do nothing.

Does it work if you add the line "memory = 0.2" to this file: drupal/modules/book/book.info (I don't know what's the really memory usage of the book module, it's just for testing)?

Sorry, after i added 'memory = 0.2' to book info, it worked just nice.
Tried it in Firefox and IE 7.0, both are working.
About not calling updateMem, it was my newby status with Firebug debugging... just learning.

In you first version, you put the estimated memory use in the line of the module. I know this is amended for space reasons. Should it not be nice to have a "tooltip" with "estimated memory use 0.2 Mb" on a checkbox mouseover?

Thanks, Frans

Why is having an estimate of module memory in .info necessary? It gets obsolete very fast between releases (e.g. someone commits a patch that consumes twice the memory, and the .info has not been updated) and will be very misleading. This is a case where not having the info is better than having wrong info.

The less we need to do things manually the better.

Let admin/build/modules show it for enabled modules only and that is it. This will be the actual memory usage.

Or, if you want to do it for unenabled modules, you can assume a base of some low number (1K?). Another alternative is to have a checkbox or link to show it for modules that are not enabled, by loading them on the spot.

Of course it would be nicer if the estimated memory in .info wasn't necessary, but I don't know a way how you could get this value automatically. If you know how, please tell me. :)

Furthermore, would it be better if the user can initial see the estimated memory use after activating the module? This would also make the warning JavaScript useless.
I think, if the user wants to activate a module, he would like to know, if drupal is still working after doing this. A wrong value in every case (1K) would be even worse than a wrong value in just some cases.

If you think the estimated memory information is a bad idea, I will remove it from the patch. It would lead to many patches in critical system parts if you wanted to check the memory increase after activating a module, also the user could only activate one with one request. Someone could write a little script that checks the memory usage automatically for any module and writes it into the .info files.

This sounds awesome. Would it be feasable to backport it to a 5.x contrib? I have a site I could really use it on that won't be 6.x for a while.... Ironically because it has so many contribs. ;)

Michelle

Phalanx

I am not sure if this will work or not, but it is worth a try.

If we start with the number of bytes in the .module, plus the number of bytes in the .inc files it calls, that should give *some* estimate.

Not without its faults, but better than leaving around obsolete information entered by humans.

The file size of the .module and its related includes is not the same as the amount of memory it will consume once parsed. However, I suspect it is a good base for what that memory usage will be. It's probably file size * some multiplier. Of course, that multiplier will vary depending on the complexity of the code, and still doesn't account for data used by the module at any given point, which is nearly impossible to predict.

If an "average" multiplier can be determined, that could be an interesting value to reference. (And of general interest for the PHP world, too.) Determining that multiplier, however, is probably out of scope for GHOP as it would require a fair bit of experimentation. It's easily a task in itself.

I agree with kbahey that an easily out-of-date value in the .info file will make matters worse, not better.

Crell

You are spot on: It is a starting point, a guesstimate if you will.

The only way to know the multiplier is by trial and error.

It will never be 100% accurate for all cases, because of data structures if nothing else.

But again, it is a starting point.

Subscribe for a backport to 5.x

I didn't expect this patch to require this amount of work.
I will remove the estimated memory if it's not needed or not wanted, but writing a function that guesses the memory amount is a very complex task, too complex for GHOP maybe. I've compared the file size versus the memory usage for the first six modules, and I can't see big relations there.

Module Name: File Size -> Memory Usage (approximate multiplier)
------------
aggregator: 92 KB -> 360 KB (x4)
blog: 16 KB -> 80 KB (x5)
blogapi: 28 KB -> 170 KB (x6)
book: 84 KB -> 250 KB (x3)
color: 24 KB -> 140 KB (x6)
comment: 108 KB -> 390 KB (x3.6)

As you can see, the differrences between file size and memory usage become smaller the bigger the modules are, but there are big fluctuations.
This is not astonishing as the memory usage doesn't increase with more code in general but with bigger use of big variables, and not if you modify them.

You see, you would have to write a parser to get a reliable value, because an outdated value in the info value would be more exact than any value generated of the file size.

PhalanX,

You are doing a great job so far.

It is quite normal for patches to take a lot of back and forth activity. This means that there is interest in it, but we (the community) wants to make it better. This is "The Drupal Way". And it is a major reason of why Drupal has an excellent architecture.

Takes some perseverance, yes, but the results are rewarding to everyone.

Hang in there ...

Wow, I love that this off-the-cuff idea created a life of its own.

I believe the estimated memory requirement should be in the .info file; and it's ok if most modules simply don't use it; most modules are fairly small. But it'd be really, really convenient for big modules like Views to be able to say "Yes I use a lot of extra memory."

Some minor comments:

memory_get_usage()/1024/1024

Operators that aren't . should always be preceded and followed by a space.

+ $checkbox .= 'onclick="'. $element['#onclick'] .'" ';

We prefer not to use onclick for javascript. Instead, in javascript we like to use $('object-css-identifier').click(function() { ...code... }))

You can use drupal_add_js() with the 'setting' to provide the information on how to do this; also, in Drupal 6 this needs to support behaviors, which will allow javascript attachment to work when using AHAH/AJAX functions (which could be valuable later). However, I fear that this change may be 'over budget' for your GHOP assignment. If it is maybe one of us who knows .js can spend a little time making that right.

Other than that, this looks really really good.

@merlinchaos

The issue is not a missing memory line in info file. I am fine with that.

The issue is obsolete information that is inaccurate, misleading and out of date. That does more harm than good. It will become obsolete quickly.

kbahey: Maybe, but I'd rather have the ability to put something there than not.

@Benjamin

The version number is not a good analogy, since it is deduced from tagging, and done pro-actively and consciously by the maintainer as they decide to add new features, fix bugs, ...etc. There is no equivalent "second nature" activity that the developer does to measure memory.

Right now, we don't have much that needs constant tender loving care. This will be one such thing.

Look at the package line. We tell people not to put anything there, so most modules (apart from views, devel, ...etc.) would end up in "Other", but people still put something there that is superfluous, and you have one field set per module.

The less we have to educate, the less trouble we face in support later ...

I'm not sure people putting the wrong value in their .info file for the package is a fair comparison.

I've implemented merlinofchaos' suggestions as far as I was able to. I had to use document.getElementById instead of $('css-identifier') anyway, because it didn't work the drupal way.

Javascript review:

$('#meminfo').html(output);

should be equivalent to:

document.getElementById('meminfo').innerHTML = output;

And is better.

Likewise:

var msgText = $('#meminfo').html().split(' ');

is better than
var msgText = document.getElementById('meminfo').innerHTML.split(' ');

This is dangerous:
var memUsage = object.parentNode.parentNode.parentNode.parentNode.childNodes[3].title;

If the HTML structure changes (as it likely will in the future) the javascript will fail for non-obvious reasons. Can we come up with a better way to tag that information? It is often possible to use string replacement on IDs to correlate one item to another.

Thanks, I didn't know about the .html() function. I've corrected it in my local version. Is there a reference by the way? I had to read the available JS files to get all that information.

I'm not submitting the changed patch yet, because I'm still thinking about this parentNode crap. I've got two ideas how to make it more save against changes:
- Going up in the DOM tree until you find a

- Counting through all the titled descriptions and selecting the right one. (A bit more complex than the other way)

Has someone another idea - or which one is the best?

There are docs on this at http://jquery.com -- .html() is a jquery function and works on jquery objects.

If you know the tag ID, you can always just $('#tag') to get the object, and do all sorts of manipulation with that.

Sounds like this is waiting on an updated patch.

I've tried to implement the second idea (finding the info by counting). The result is a much more complex method, that should be more flexible to code changes. It is not very clean in my eyes, but I don't know how to improve it.

This is the function that gets the memory usage of a specific module with the clicked checkbox as argument:

Drupal.getMemoryUsage = function (object) {
  var checkboxes = $('.package input.form-checkbox').get();
  var thisRow = -1;

  // search this object
  for (var i = 0; i < checkboxes.length; ++i) {
    if (checkboxes[i] == object) {
      thisRow = i;
      break;
    }
  }

  // get the tag with the description at the found index
  var memUsage = $('.package *[@title]').map(function(){return this.title;}).get(thisRow);
  if (!memUsage) return 0.0;

  // find first number: the memory usage of the module
  memUsage = memUsage.split(' ');
  for (var i = 0; i < memUsage.length; ++i) {
    if (parseFloat(memUsage[i]) > 0 && parseFloat(memUsage[i]) < Infinity) {
      return parseFloat(memUsage[i]);
    }
  }
  return -1.0;
};

I'll post my current state of the patch, because I won't be able to work on it the next three days.
It isn't perfect, but it works (at least on my local version).

I don't have any better ideas about how to get the memory usage, so while not completely clean I am okay with this.

I have only one small code style comment left on the javascript:

var memUsage = $('.package *[@title]').map(function(){ return this.title; }).get(thisRow);

In the PHP, I'd like to see a bit more documentation about the math being used in system_memory_info()

Otherwise, I'm satisfied with this. I'm only reviewing the javascript part; the rest can be handled by people with more time, but I know that the .js knowledge isn't as available. So if this passes functional testing by another user, I'd say this is RTBC.

(Note: the above changes can be made by anybody, since PhalanX is not available for the next 3 days, the patch should be easy enough to just reroll).

Okay, I've done the changes now and I tried to simplify the JavaScript by using regular expressions.

@PhalanX: I'm sorry this has sat here for 3 days without review. I am a horrible person to review this patch, because I know nothing about javascript and have not been following this task, but here goes :)

I installed the patch onto a fresh install of D6-HEAD (just after the RC1 announcement). The patch applied with offset.

If I enable or disable modules and then click the save configuration button, the memory usage message increases or decreases.

However, when I click on individual enabled checkboxes for modules, nothing happens (other than the check appearing or disappearing). I do notice that when using Firebug each time I check or uncheck the enabled box for a module, I get another instance of this error message in the Firefox debug console:

/([\d.]+)/.exec(memUsage) has no properties
getMemoryUsage(input#edit-status-blogapi.form-checkbox blogapi)memorywarn.js (line 44)
memoryWarn()memorywarn.js (line 5)
e(Object type=click)jquery.js (line 13)
e()jquery.js (line 13)
[Break on this error] return parseFloat((/([\d.]+)/.exec(memUsage)[1]));

I would expect that something should happen when I click the enabled checkbox, but I'm not sure. In any case, I wouldn't expect these errors.

Hovering over the description text for a module for which I have added "memory = 0.2" into the .info file results in a tool tip displaying the amount of memory used by that module.

Unless someone else speaks up first, if you can either: a)Post a patch which fixes the javascript error I described above, or b) convince me that the error I described above is expected behavior, I will give you credit for this task, given merlin's comments in #42 above.

By the way, you'll have much better luck getting reviews if you enter the #drupal or #drupal-ghop IRC channels on irc.freenode.net and ask people to review the patch.

Thanks for hanging in there with this one.

I've reviewed the patch. Of course the error is not a wanted behavior. I'm sorry I didn't check this myself.

I made the patch for the version in CVS, so it's possible that the line numbers aren't correct in Drupal HEAD.

Here is my new version:

I think this should be ready for review.

I applied the patch in #45 and the errors when I click/unclick enable button for modules is gone. However, when I do so nothing seems to happen as far as the memory display. Should something happen then?

Another thing I found strange: I tried adding this line to book.info:

memory =200.2

The allowed memory in my php.ini is 48 MB. If I enable the book module and submit my changes, I get this:

Memory usage: 9.37 of 48 MB.

if I then disable the book module again I get this:

Memory usage: 9.13 of 48 MB.

So is the point of adding a memory line to the .info file only to provide a number that is displayed in the tooltip? And if I specify that a module uses a lot of memory, shouldn't I get a warning *before* I submit the changes which will likely cause a WSOD?

If what I report above is the expected behavior of this patch then let me know. There don't seem to be any errors this time, so if this behavior is what the previous reviewers have decided upon, then I think you're done as far as the GHOP issue goes. Otherwise the patch might need a little more work.

Ok, in that case I'll give you credit for this GHOP task. Nice job.

As per Earl's request, I have taken a look at the JS included in this patch and can say that there are no objections from my side.

Something is still not right.

Installing a default Drupal HEAD, this patch says 3.6 MB are used. Yet, my memory display patch mentioned earlier (see followup #1 above) shows that the total is 3.29 MB (3452256 bytes to be exact).

I enable aggregator, and mine shows 49392 bytes, yours show 3.82 MB (0.22 MB, not just 49K).

I enable forum, and mine shows 35112 bytes, yours show 3.95 MB (0.13 MB not just 35K)

Don't know why these discrepancies are there, they need to be looked at.

Also, when disabling modules that I enabled (forum and aggregator in this case), I get this:

Memory usage: -0.98 of 128 MB.

Which is negative, and hence incorrect. This is again because of the estimate in the info file, which will cause confusion if not accurate (and will be so often).

It seems that when you uncheck a module, the memory usage line does subtract the estimate, not the actual, and hence we come to a negative if the estimate is not correct.

As for the discrepancy in numbers, I can't see how calling the same function would return different values. The patch is available in #1, so someone can apply it and verify.

The patch SHOULD work like this (it does in my local version):
- the line "Memory usage: 6 of 10 MB" uses a built-in PHP function that returns the current memory usage. This should return the correct result (at least for the moment it's called, maybe it's higher if the page is completely loaded).
- There is a new parameter in the module info files that will be checked and applied as title to the module descriptions.
- If the user activates a module checkbox, the JS is activated. It adds the estimated memory usage to the current one. If there are less than 5 MB remaining, it lets the user confirm the action.

Reasons for your problems could be:
- different memory usage results: Your call to the PHP function is done earlier, so there was something put into memory after yours and before mine.
- negative memory usage: You are deactivating a module whose estimated memory usage is wrong:
-- an example: Memory usage: 7 MB; Module memory usage: 7.98 MB => -0.98 MB remaining.

Moving feature requests to D7 queue. Sorry...

Also, when disabling modules that I enabled (forum and aggregator in this case), I get this:

Memory usage: -0.98 of 128 MB.

Which is negative, and hence incorrect. This is again because of the estimate in the info file, which will cause confusion if not accurate (and will be so often).

What about doing max($memory_usage, 0); and avoiding the confusion?

Subscribe for a backport to 5.x

I would like to see the "Show how much memory is in use" issue de-coupled from allowing modules to specify a memory requirement in their .info files (which will auto-bump if possible, else fail to install... see some of the earlier discussion at #295697: Increase PHP Memory Limit for Simpletest).

If you're interested in working on this, please see #309457: Allow profiles to specify required memory in .info file.

Marking postponed until then.

devel.module shows memory usage, is this patch still needed or wanted?

Also, I find memory problems are often caused by Drupals internal (code) cache, so if you enable N modules to stay under your memory limit but then call node_load() 200 times, you'll still run out and get a WSOD.

N.B, #309457: Allow profiles to specify required memory in .info file is rejected.